Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions ReviewSLA/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ Bot PRs and Graphite merge-queue PRs have no clocks.
and late scheduled runs don't send them twice. The comment is written before the DM, so a failed
write means a missed reminder, not a repeated one.
- **Reminder DMs** (`remind`): one per clock, when it's due, to the assignee's Slack ID in
`REVIEW_PEOPLE`.
`REVIEW_PEOPLE`, linking to the PR in Graphite.
- **Out-of-office reassignment** (`remind`): an assignee the PTO calendar has out on any day from
today to the day their review is due is replaced on every PR in the stack they're assigned to
and haven't approved. Their approvals stand.
Expand All @@ -54,10 +54,12 @@ Bot PRs and Graphite merge-queue PRs have no clocks.
kept, and a comment on the lowest of those PRs says who took over and why. In a public repo it
says only that they aren't available, since anyone can read it.
- **The digest:** posted to the `REVIEW_SLA_SLACK_CHANNEL` channel by the 10:00 Pacific run
(or a manual run with **digest** ticked): overdue reviews by person, and how many more are due
today.
(or a manual run with **digest** ticked): the overdue reviews and the ones due today, by person,
each linked to the PR in Graphite, and how many more are owed but due later.
- **The dashboard:** a Confluence page with a checkmark per person while they're meeting the SLA,
and their overdue reviews when they aren't. It saves a new version only when the content
and the number overdue when they aren't. Next to it, every review they owe, linked to the PR in
Graphite, with when it's due, overdue ones in bold. PR titles are left out so a busy row stays
short; the link leads to the PR. It saves a new version only when the content
changes, as a minor edit, so watchers aren't notified every 30 minutes.

It fails safe: an API error is a warning, and that PR, repo, channel or page is skipped this run.
Expand Down
9 changes: 8 additions & 1 deletion ReviewSLA/comment.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ function formatTime(instant) {
return `${parts.weekday} ${parts.month} ${parts.day}, ${parts.hour}:${parts.minute}`;
}

// A PR's page in Graphite, where the team reviews stacks: github.com/<owner>/<repo>/pull/<n> becomes
// app.graphite.com/github/pr/<owner>/<repo>/<n>.
function graphiteUrl(pr) {
const match = /^https:\/\/github\.com\/([^/]+)\/([^/]+)\/pull\/(\d+)/.exec(pr.url ?? '');
return match ? `https://app.graphite.com/github/pr/${match[1]}/${match[2]}/${match[3]}` : pr.url;
}

// One key per clock, so a reminder is sent once per clock: a new clock (a re-review, or a restart
// after the PR goes back to draft) has a new start and gets its own.
const reminderKey = (clock) => `${clock.login}:${clock.kind}:${clock.start.toISOString()}`;
Expand Down Expand Up @@ -60,4 +67,4 @@ function renderComment(clocks, state, { dashboardUrl } = {}) {
return lines.join('\n');
}

module.exports = { formatTime, KIND, MARKER, parseState, reminderKey, renderComment };
module.exports = { formatTime, graphiteUrl, KIND, MARKER, parseState, reminderKey, renderComment };
25 changes: 14 additions & 11 deletions ReviewSLA/confluence.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
// The Review SLA dashboard: a Confluence page under Embedded Team Dashboard, rewritten each run.
// It shows a checkmark per person while they're meeting the SLA, and lists overdue reviews only for
// those who aren't. A new version is saved only when the content changes, as a minor edit, so
// It shows a checkmark per person while they're meeting the SLA, and the number overdue when they
// aren't, then every review each person owes, linked to Graphite, with when it's due. A new version is saved only when the content changes, as a minor edit, so
// neither the page history nor watchers' notifications fill up every 30 minutes. The version
// message carries a hash of the content, which is what "changed" is checked against: Confluence
// rewrites storage format on save, so the page's own body can't be compared.

const crypto = require('node:crypto');
const { formatTime, KIND } = require('./comment.js');
const { formatTime, graphiteUrl, KIND } = require('./comment.js');
const { SMALL_PR_LINES, TARGET_HOURS } = require('./clock.js');

const html = (text) => String(text).replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;').replace(/"/g, '&quot;');
Expand All @@ -15,19 +15,22 @@ function renderDashboard(clocks, { people }) {
const nameOf = (login) => people[login]?.name || login;
const logins = [...new Set([...Object.keys(people), ...clocks.map((c) => c.login)])].sort((a, b) => nameOf(a).localeCompare(nameOf(b)));
const rows = logins.map((login) => {
const mine = clocks.filter((c) => c.login === login);
const overdue = mine.filter((c) => c.overdue).sort((a, b) => a.due - b.due);
const status = overdue.length === 0 ? '✅' : `${overdue.length} overdue`;
const list = overdue.length === 0 ? '' : `<ul>${overdue
.map((c) => `<li><a href="${html(c.pr.url)}">${html(`${c.pr.repo}#${c.pr.number}`)}</a> ${html(c.pr.title)}: ${KIND[c.kind].toLowerCase()}, overdue since ${html(formatTime(c.due))}</li>`)
.join('')}</ul>`;
return `<tr><td>${html(nameOf(login))}</td><td>${status}</td><td>${mine.length}</td><td>${list}</td></tr>`;
const mine = clocks.filter((c) => c.login === login).sort((a, b) => a.due - b.due);
const overdue = mine.filter((c) => c.overdue).length;
const status = overdue === 0 ? '✅' : `${overdue} overdue`;
const item = (c) => {
const when = c.overdue ? `<strong>overdue since ${html(formatTime(c.due))}</strong>` : `due ${html(formatTime(c.due))}`;
// No title: the link leads to it, and titles make a busy row too tall to scan.
return `<li><a href="${html(graphiteUrl(c.pr))}">${html(`${c.pr.repo}#${c.pr.number}`)}</a>: ${KIND[c.kind].toLowerCase()}, ${when}</li>`;
};
const list = mine.length === 0 ? '' : `<ul>${mine.map(item).join('')}</ul>`;
return `<tr><td>${html(nameOf(login))}</td><td>${status}</td><td>${list}</td></tr>`;
});
return [
'<p>Kept up to date by the Review SLA job every 30 minutes during business hours (10:00 to 17:00 Pacific, business days). ' +
`An assignee owes a first response within ${TARGET_HOURS.small} business hours on a PR under ${SMALL_PR_LINES} added lines, ` +
`${TARGET_HOURS.standard} otherwise, and a re-review within ${TARGET_HOURS.reReview}. Times are Pacific.</p>`,
'<table><tbody><tr><th>Assignee</th><th>Status</th><th>Reviews owed</th><th>Overdue</th></tr>',
'<table><tbody><tr><th>Assignee</th><th>Status</th><th>Reviews owed</th></tr>',
...rows,
'</tbody></table>',
].join('');
Expand Down
45 changes: 43 additions & 2 deletions ReviewSLA/run.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ test('remind DMs each overdue assignee once, however many runs see it', async ()
assert.deepStrictEqual(w.calls.dms, []);
await go(w, { now: '2026-10-02T00:00:00Z' });
assert.deepStrictEqual(w.calls.dms.map((m) => m.channel).sort(), ['U000', 'U002']);
assert.match(w.calls.dms[0].text, /^Review due: <https:\/\/github.com\/StaflSystems\/StaflLib\/pull\/1\|StaflLib#1: PR 1>\. As an assignee, your first response was due at Thu Oct 1, 17:00 PT\.$/);
assert.match(w.calls.dms[0].text, /^Review due: <https:\/\/app.graphite.com\/github\/pr\/StaflSystems\/StaflLib\/1\|StaflLib#1: PR 1>\. As an assignee, your first response was due at Thu Oct 1, 17:00 PT\.$/);
await go(w, { now: '2026-10-02T17:30:00Z' });
assert.strictEqual(w.calls.dms.length, 2);
// One comment, edited in place.
Expand Down Expand Up @@ -248,11 +248,52 @@ test('the digest posts only on the 10:00 Pacific run', async () => {
await go(w, { now: '2026-10-02T17:05:00Z', inputs: { schedule: '0 17 * * 1-5' } });
assert.strictEqual(w.calls.posts.length, 1);
assert.strictEqual(w.calls.posts[0].channel, 'C123');
assert.match(w.calls.posts[0].text, /^\*Review SLA, Fri Oct 2\*\n2 overdue reviews:\n• Person ghopper: /);
assert.strictEqual(
w.calls.posts[0].text,
[
'*Review SLA, Fri Oct 2*',
'*Overdue (2)*',
'• Person ghopper: <https://app.graphite.com/github/pr/StaflSystems/StaflLib/1|StaflLib#1: PR 1>, first response due Thu Oct 1, 17:00',
'• Person kjohnson: <https://app.graphite.com/github/pr/StaflSystems/StaflLib/1|StaflLib#1: PR 1>, first response due Thu Oct 1, 17:00',
'<https://example.atlassian.net/wiki/spaces/EM/pages/42|Dashboard>',
].join('\n'),
);
assert.ok(run.isDigestRun({ eventName: 'schedule', schedule: '0 18 * * 1-5' }, new Date('2026-12-01T18:00:00Z')));
assert.ok(run.isDigestRun({ digest: 'true', eventName: 'workflow_dispatch' }, new Date('2026-12-01T20:00:00Z')));
});

test('the digest also lists reviews due today, and counts the ones due later', async () => {
// PR 1 opened 10:00 Thursday is due 17:00 Thursday; PR 2, opened 16:00 Thursday, isn't due until Friday.
const second = { number: 2, head: 'feature/b', assignees: ['mhamilton'], timeline: [{ __typename: 'AssignedEvent', createdAt: '2026-10-01T23:00:00Z', assignee: userNode('mhamilton') }] };
const w = world({ prs: [onePr(), second] });
await go(w, { now: '2026-10-01T23:30:00Z', inputs: { digest: 'true' } });
assert.strictEqual(
w.calls.posts[0].text,
[
'*Review SLA, Thu Oct 1*',
'*Due today (2)*',
'• Person ghopper: <https://app.graphite.com/github/pr/StaflSystems/StaflLib/1|StaflLib#1: PR 1>, first response due 17:00',
'• Person kjohnson: <https://app.graphite.com/github/pr/StaflSystems/StaflLib/1|StaflLib#1: PR 1>, first response due 17:00',
'1 more review is owed, due after today.',
'<https://example.atlassian.net/wiki/spaces/EM/pages/42|Dashboard>',
].join('\n'),
);
});

test('the dashboard lists every review owed, linked to Graphite, with overdue ones marked', async () => {
const second = { number: 2, head: 'feature/b', assignees: ['ghopper'], timeline: [{ __typename: 'AssignedEvent', createdAt: '2026-10-01T23:00:00Z', assignee: userNode('ghopper') }] };
const w = world({ prs: [onePr(), second] });
await go(w, { now: '2026-10-02T00:00:00Z' });
const page = w.calls.puts.at(-1).body.storage.value;
assert.ok(page.includes(
'<tr><td>Person ghopper</td><td>1 overdue</td><td><ul>' +
'<li><a href="https://app.graphite.com/github/pr/StaflSystems/StaflLib/1">StaflLib#1</a>: first response, <strong>overdue since Thu Oct 1, 17:00</strong></li>' +
'<li><a href="https://app.graphite.com/github/pr/StaflSystems/StaflLib/2">StaflLib#2</a>: first response, due Fri Oct 2, 16:00</li>' +
'</ul></td></tr>',
), page);
assert.ok(page.includes('<tr><td>Person dvaughan</td><td>✅</td><td></td></tr>'));
});

test('the dashboard saves a new version only when its content changes, as a minor edit', async () => {
const w = world({ prs: [onePr()] });
await go(w, { now: '2026-10-01T18:00:00Z' });
Expand Down
26 changes: 17 additions & 9 deletions ReviewSLA/slack.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// is chat:write, and the bot has to be invited to the digest channel.

const { pacificDate } = require('../ReviewConfig/pto.js');
const { formatTime, KIND } = require('./comment.js');
const { formatTime, graphiteUrl, KIND } = require('./comment.js');

async function postMessage({ token, channel, text, fetch = globalThis.fetch }) {
const response = await fetch('https://slack.com/api/chat.postMessage', {
Expand All @@ -16,24 +16,32 @@ async function postMessage({ token, channel, text, fetch = globalThis.fetch }) {

// Slack's mrkdwn wants &, < and > escaped in text.
const escape = (text) => text.replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;');
const prLink = (pr) => `<${pr.url}|${pr.repo}#${pr.number}: ${escape(pr.title)}>`;
const prLink = (pr) => `<${graphiteUrl(pr)}|${pr.repo}#${pr.number}: ${escape(pr.title)}>`;

function reminderText(clock) {
const what = clock.kind === 'first' ? 'your first response' : 'your re-review';
return `Review due: ${prLink(clock.pr)}. As an assignee, ${what} was due at ${formatTime(clock.due)} PT.`;
}

// Overdue reviews and those due today, by person, each linked to Graphite, then a count of the rest.
function digestText(clocks, { now, people, dashboardUrl }) {
const nameOf = (login) => people[login]?.name || login;
const overdue = clocks.filter((c) => c.overdue).sort((a, b) => nameOf(a.login).localeCompare(nameOf(b.login)) || a.due - b.due);
const dueToday = clocks.filter((c) => !c.overdue && pacificDate(c.due) === pacificDate(now));
const byPerson = (a, b) => nameOf(a.login).localeCompare(nameOf(b.login)) || a.due - b.due;
const overdue = clocks.filter((c) => c.overdue).sort(byPerson);
const dueToday = clocks.filter((c) => !c.overdue && pacificDate(c.due) === pacificDate(now)).sort(byPerson);
const later = clocks.length - overdue.length - dueToday.length;
const line = (c, when) => `• ${escape(nameOf(c.login))}: ${prLink(c.pr)}, ${KIND[c.kind].toLowerCase()} due ${when}`;
const lines = [`*Review SLA, ${formatTime(now).split(',')[0]}*`];
if (overdue.length === 0) lines.push('No overdue reviews.');
else {
lines.push(`${overdue.length} overdue ${overdue.length === 1 ? 'review' : 'reviews'}:`);
for (const c of overdue) lines.push(`• ${escape(nameOf(c.login))}: ${prLink(c.pr)} (${KIND[c.kind].toLowerCase()}, due ${formatTime(c.due)})`);
if (overdue.length === 0 && dueToday.length === 0) lines.push('No reviews overdue or due today.');
if (overdue.length > 0) {
lines.push(`*Overdue (${overdue.length})*`);
for (const c of overdue) lines.push(line(c, formatTime(c.due)));
}
if (dueToday.length > 0) lines.push(`${dueToday.length} more due today.`);
if (dueToday.length > 0) {
lines.push(`*Due today (${dueToday.length})*`);
for (const c of dueToday) lines.push(line(c, formatTime(c.due).split(', ')[1]));
}
if (later > 0) lines.push(`${later} more ${later === 1 ? 'review is' : 'reviews are'} owed, due after today.`);
if (dashboardUrl) lines.push(`<${dashboardUrl}|Dashboard>`);
return lines.join('\n');
}
Expand Down
Loading