From ef4a4146f7c5760a535fb6bb26c24b3c19d151b7 Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Tue, 29 Sep 2026 16:33:21 -0700 Subject: [PATCH 1/4] Repair missed webhooks every hour A PR whose close webhook gets lost stays open on the board until the next restart, because #501 repairs it only at startup. Every backlog number counts those PRs: on 2026-09-29 the board listed 314 open PRs, and 67 of them were already merged or closed on GitHub. Now the server lists each tracked repo's open pulls once an hour and refreshes only the ones the DB has wrong: open on GitHub but not open in the DB (a lost `opened` or `reopened`), and open in the DB but missing from the listing (a lost `closed`, repaired by #501's own refreshStaleOpenPulls). The listing costs one API call per 30 open pulls in each repo, and an hour with nothing wrong refreshes nothing. This is option 1 from #501's list, which went with startup only. The startup refresh still refetches every open pull; this doesn't. Co-Authored-By: Claude Opus 5.5 --- app.js | 11 ++++++++ lib/refresh.js | 53 +++++++++++++++++++++++++++++++++++ test/stale-open-pulls.test.js | 10 ++++++- 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/app.js b/app.js index 42bf37c7..beb745fd 100644 --- a/app.js +++ b/app.js @@ -99,6 +99,17 @@ dbManager }) .done(); +// Webhooks get lost, and a lost `closed` left a PR open on the board until the +// next restart (pulldasher#501 repairs it only at startup), which inflates every +// backlog number. Once an hour, list each repo's open pulls and refresh just the +// ones the DB has wrong. +const RECONCILE_MS = 60 * 60 * 1000; +setInterval(function () { + refresh.reconcileOpenPulls().catch(function (err) { + console.error('Hourly open-pull repair failed: %s', (err && err.message) || err); + }); +}, RECONCILE_MS); + //==================================================== // Socket.IO const io = new Server(httpServer); diff --git a/lib/refresh.js b/lib/refresh.js index e09fec5e..fbd78677 100644 --- a/lib/refresh.js +++ b/lib/refresh.js @@ -133,6 +133,49 @@ export function createRefresh({ pacer = noopPacer } = {}) { ) ); }, + + /** + * The cheap repair for lost webhooks: list each repo's open pulls + * (about one API call per repo, per 30 pulls) and refresh only the ones + * the DB has wrong. Open on GitHub but not in the DB means a lost + * `opened` or `reopened`; open in the DB but gone from the listing means + * a lost `closed`. openPulls repairs the same things but refetches every + * open pull on the way, which costs thousands of calls, so it runs only + * at startup. + */ + reconcileOpenPulls: async function reconcileOpenPulls(repos) { + refreshDebug('reconcile open pulls'); + const listedRepos = []; + const listing = await utils.forEachRepo( + repo => + gitManager.getOpenPulls(repo, pacer).then(pulls => { + listedRepos.push(repo); + return pulls; + }), + { repos: repos } + ); + let missing = []; + try { + missing = findMissingOpenPulls(await dbManager.getOpenPullIds(), listing.items); + } catch (err) { + console.error('Failed to load open pulls from the DB: %s', (err && err.message) || err); + } + refreshDebug('refreshing %s pulls open on GitHub but not in the DB', missing.length); + const report = await drainThrough(pullQueue)({ + items: missing, + failedRepos: listing.failedRepos, + }); + const staleFailures = await refreshStaleOpenPulls( + pullQueue, + pacer, + listing.items, + listedRepos + ); + return { + failedRepos: report.failedRepos, + failedItems: report.failedItems.concat(staleFailures), + }; + }, }; } @@ -335,6 +378,16 @@ export function findStaleOpenPulls(dbOpenPulls, listedPulls, listedRepos) { ); } +/** + * The listed open pulls the DB doesn't hold as open: new or reopened pulls + * whose webhook never arrived. Repo names compare case-insensitively. + */ +export function findMissingOpenPulls(dbOpenPulls, listedPulls) { + const key = (repo, number) => repo.toLowerCase() + '#' + number; + const known = new Set(dbOpenPulls.map(pull => key(pull.repo, pull.number))); + return listedPulls.filter(pull => !known.has(key(pull.base.repo.full_name, pull.number))); +} + /** * A closed pull never appears in the open-pulls listing, so one whose close webhook * was lost stays open in the DB until refetched. Never rejects: app.js ignores it. diff --git a/test/stale-open-pulls.test.js b/test/stale-open-pulls.test.js index fea759a6..f3876120 100644 --- a/test/stale-open-pulls.test.js +++ b/test/stale-open-pulls.test.js @@ -2,7 +2,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import gitManager from "../lib/git-manager.js"; import dbManager from "../lib/db-manager.js"; -import { createRefresh, findStaleOpenPulls } from "../lib/refresh.js"; +import { createRefresh, findMissingOpenPulls, findStaleOpenPulls } from "../lib/refresh.js"; const githubPull = (repo, number) => ({ number, base: { repo: { full_name: repo } } }); @@ -26,6 +26,14 @@ test("findStaleOpenPulls keeps DB-open pulls missing from a successful listing", ]); }); +test("findMissingOpenPulls keeps listed pulls the DB doesn't hold as open", () => { + const listed = [githubPull("Test/Repo-A", 1), githubPull("test/repo-a", 2)]; + + const missing = findMissingOpenPulls([{ repo: "test/repo-a", number: 1 }], listed); + + assert.deepEqual(missing, [githubPull("test/repo-a", 2)]); +}); + // A pull whose close webhook was lost stays open in the DB. openPulls refetches it, // skipping repos whose listing failed and reporting a refetch that fails. test("openPulls refetches pulls the DB holds as open that GitHub no longer lists", async (t) => { From bcf6d1c26e415248a3bd49953856fb52bb42d3af Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Fri, 2 Oct 2026 16:20:44 -0700 Subject: [PATCH 2/4] Fetch a pull's checks in one call A full refresh of an iFixit/ifixit pull makes about 21 GitHub calls, and most of them list jobs: one call per workflow run on the head commit. On 15 open pulls that was 176 of 314 calls. Refresh all runs a full refresh for every open pull, so this is most of what a press costs. checks.listForRef lists every check run on the commit in one call. 1accc56c moved off it in 2023 because fine-grained tokens can't call it. GitHub's list of the endpoints they can call still leaves it out: https://docs.github.com/en/rest/authentication/endpoints-available-for-fine-grained-personal-access-tokens Nobody could say which kind of token the bot has, so the first 403 saying "not accessible" switches the process back to runs and jobs. Two smaller savings: the pull already has its labels, so the issues.get call is gone, and every list asks for 100 items a page. Measured with a personal token on the same pulls, old code then new: repo (pulls sampled) calls per pull ms per pull ifixit (15) 20.9 to 8.0 1134 to 557 ops (3) 17.7 to 8.0 1120 to 596 fixbot (3) 19.3 to 8.0 1480 to 538 ifixit-schooner-fw (3) 17.0 to 8.0 1451 to 513 server-templates (3) 14.0 to 8.0 1048 to 514 Product-Development (3) 10.3 to 8.0 792 to 518 ifixit-cli (3) 13.0 to 8.0 1374 to 528 With check runs refused, iFixit/ifixit takes 20.1 calls. On all 33 pulls the board gets the same checks, labels, comments and reviews. Three pulls' CI moved between the two runs; listed at one moment, check runs and jobs agreed on all 22, 24 and 47 of their checks. Note: check runs include checks from apps other than Actions, which the jobs path never read. None of the 33 pulls has one, and the check_run webhook already saves them. Co-Authored-By: Claude Opus 5.5 --- lib/git-manager.js | 68 +++++++++++--- test/git-manager-refresh.test.js | 153 +++++++++++++++++++++++++++++++ 2 files changed, 206 insertions(+), 15 deletions(-) create mode 100644 test/git-manager-refresh.test.js diff --git a/lib/git-manager.js b/lib/git-manager.js index 6e482006..534aae97 100644 --- a/lib/git-manager.js +++ b/lib/git-manager.js @@ -311,10 +311,8 @@ export default { var comments = getIssueComments(repo, githubPull.number); var headCommit = getCommit(repo, githubPull.head.sha); var commitStatuses = getCommitStatuses(repo, githubPull.head.sha); - var jobRuns = getAllJobRuns(repo, githubPull.head.sha); + var jobRuns = getCheckRuns(repo, githubPull.head.sha); var events = getIssueEvents(repo, githubPull.number); - // Only so we have the canonical list of labels. - var ghIssue = getIssue(repo, githubPull.number); var reviews = getReviews(repo, githubPull.number); // Resolved (and cached) once per process; threaded through so // deriveReviewRequests can tell a pulldasher claim apart from a @@ -330,7 +328,6 @@ export default { commitStatuses, jobRuns, events, - ghIssue, reviews, botLogin, diffStats, @@ -341,10 +338,9 @@ export default { commitStatuses = results[3], jobRuns = results[4], events = results[5], - ghIssue = results[6], - reviews = results[7], - botLogin = results[8], - fullPull = results[9]; + reviews = results[6], + botLogin = results[7], + fullPull = results[8]; // graft the fields a list item lacks (diff stats + mergeable, see // diffStats above). Omitting mergeable here re-writes it as NULL on @@ -454,8 +450,13 @@ export default { let allCommitStatuses = statuses.concat(checks); - // Array of Label objects. - const labels = getLabelsFromEvents(events, ghIssue); + // Array of Label objects. The pull carries the same labels as its + // issue, so there's no issues.get call for them. + const labels = getLabelsFromEvents(events, { + number: githubPull.number, + repo: repo, + labels: githubPull.labels, + }); // Array of { login, at, self }, one per current requested_reviewers // entry -- see deriveReviewRequests. @@ -709,7 +710,10 @@ function parseRepo(repo) { */ function getIssueEvents(repo, number) { return logErrors( - github.paginate(githubRest.issues.listEvents, params({ issue_number: number }, repo)), + github.paginate( + githubRest.issues.listEvents, + params({ issue_number: number, per_page: 100 }, repo) + ), 'Getting events for issue %s:%s', repo, number @@ -718,7 +722,10 @@ function getIssueEvents(repo, number) { function getIssueComments(repo, number) { return logErrors( - github.paginate(githubRest.issues.listComments, params({ issue_number: number }, repo)), + github.paginate( + githubRest.issues.listComments, + params({ issue_number: number, per_page: 100 }, repo) + ), 'Getting comments for issue %s:%s', repo, number @@ -727,7 +734,10 @@ function getIssueComments(repo, number) { function getReviews(repo, number) { return logErrors( - github.paginate(githubRest.pulls.listReviews, params({ pull_number: number }, repo)), + github.paginate( + githubRest.pulls.listReviews, + params({ pull_number: number, per_page: 100 }, repo) + ), 'Getting reviews for pull %s:%s', repo, number @@ -736,7 +746,10 @@ function getReviews(repo, number) { function getPullReviewComments(repo, number) { return logErrors( - github.paginate(githubRest.pulls.listReviewComments, params({ pull_number: number }, repo)), + github.paginate( + githubRest.pulls.listReviewComments, + params({ pull_number: number, per_page: 100 }, repo) + ), 'Getting pull review comments for pull %s:%s', repo, number @@ -755,7 +768,7 @@ function getCommit(repo, sha) { function getCommitStatuses(repo, ref) { return logErrors( githubRest.repos - .getCombinedStatusForRef(params({ ref }, repo)) + .getCombinedStatusForRef(params({ ref, per_page: 100 }, repo)) .then(res => res.data.statuses) .then(statuses => statuses || []), 'Getting commit status for %s:%s', @@ -764,6 +777,28 @@ function getCommitStatuses(repo, ref) { ); } +// One call lists every check run on the head commit. A fine-grained token +// can't read check runs (1accc56c moved off this call for that), so after the +// first refusal this process lists each workflow run's jobs instead, about 13 +// calls per iFixit/ifixit pull. +let checkRunsRefused = false; + +function getCheckRuns(repo, ref) { + if (checkRunsRefused) { + return getAllJobRuns(repo, ref); + } + return github + .paginate(githubRest.checks.listForRef, params({ ref, per_page: 100 }, repo)) + .catch(err => { + if (err.status !== 403 || !/not accessible/i.test(err.message)) { + throw err; + } + checkRunsRefused = true; + gitDebug('Check runs refused (%s), listing workflow jobs instead', err.message); + return getAllJobRuns(repo, ref); + }); +} + function getAllJobRuns(repo, ref) { return logErrors( github @@ -773,6 +808,7 @@ function getAllJobRuns(repo, ref) { { head_sha: ref, exclude_pull_requests: true, + per_page: 100, }, repo ) @@ -793,6 +829,7 @@ function getJobRunsFromWorkflow(workflowRun) { params( { run_id: workflowRun.id, + per_page: 100, }, workflowRun.repository.full_name ) @@ -854,3 +891,4 @@ function hasNextPage(response) { const link = response.headers && response.headers.link; return Boolean(link) && link.includes('rel="next"'); } + diff --git a/test/git-manager-refresh.test.js b/test/git-manager-refresh.test.js new file mode 100644 index 00000000..b4e55859 --- /dev/null +++ b/test/git-manager-refresh.test.js @@ -0,0 +1,153 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import gitManager from "../lib/git-manager.js"; + +// Stand in for api.github.com: each route answers by path (and, for GraphQL, +// by query), and every call made is recorded as "METHOD /path". +function fakeGithub(t, routes) { + const calls = []; + t.mock.method(globalThis, "fetch", async (url, init = {}) => { + const { pathname } = new URL(typeof url === "string" ? url : url.url); + const method = init.method || "GET"; + calls.push(`${method} ${pathname}`); + const route = routes.find(([pattern]) => pattern.test(pathname)); + const { status = 200, body } = route + ? route[1](init.body ? JSON.parse(init.body) : null) + : { status: 404, body: { message: "Not Found" } }; + return new Response(JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }); + }); + return calls; +} + +const repo = "/repos/test/repo-a"; +const job = { + name: "phpunit", + status: "completed", + conclusion: "success", + html_url: "https://github.com/test/repo-a/actions/runs/77/job/9", + started_at: "2026-09-20T10:02:00Z", + completed_at: "2026-09-20T10:09:00Z", +}; + +function parseRoutes(checkRuns) { + return [ + [/^\/user$/, () => ({ body: { login: "pulldasher-bot" } })], + [/\/pulls\/3\/comments$/, () => ({ body: [] })], + [/\/issues\/3\/comments$/, () => ({ body: [] })], + [ + /\/commits\/abc$/, + () => ({ + body: { commit: { committer: { date: "2026-09-20T10:00:00Z" } } }, + }), + ], + [/\/commits\/abc\/status$/, () => ({ body: { statuses: [] } })], + [/\/commits\/abc\/check-runs$/, checkRuns], + [ + /\/actions\/runs$/, + () => ({ + body: { + total_count: 1, + workflow_runs: [{ id: 77, repository: { full_name: "test/repo-a" } }], + }, + }), + ], + [ + /\/actions\/runs\/77\/jobs$/, + () => ({ body: { total_count: 1, jobs: [job] } }), + ], + [ + /\/issues\/3\/events$/, + () => ({ + body: [ + { + event: "labeled", + label: { name: "size: S" }, + actor: { login: "labeler" }, + created_at: "2026-09-20T10:01:00Z", + }, + ], + }), + ], + [/\/pulls\/3\/reviews$/, () => ({ body: [] })], + ]; +} + +// A pulls.get response, so parse fetches no diff stats. +const githubPull = () => ({ + number: 3, + state: "open", + title: "t", + body: "", + draft: false, + created_at: "2026-09-19T00:00:00Z", + updated_at: "2026-09-20T10:01:00Z", + user: { login: "author" }, + labels: [{ name: "size: S" }], + requested_reviewers: [], + assignees: [], + milestone: null, + head: { + ref: "f", + sha: "abc", + repo: { name: "repo-a", owner: { login: "test" } }, + }, + base: { ref: "master", repo: { full_name: "test/repo-a" } }, + additions: 1, + deletions: 1, + changed_files: 1, + mergeable: true, +}); + +test("parse reads the checks in one call and the labels from the pull itself", async (t) => { + const calls = fakeGithub( + t, + parseRoutes(() => ({ body: { total_count: 1, check_runs: [job] } })) + ); + + const pull = await gitManager.parse(githubPull()); + + assert.deepEqual( + pull.commitStatuses.map((s) => `${s.data.context}=${s.data.state}`), + ["phpunit=success"] + ); + assert.deepEqual( + pull.labels.map((l) => `${l.data.title} by ${l.data.user}`), + ["size: S by labeler"] + ); + assert.ok( + !calls.includes(`GET ${repo}/issues/3`), + "no issues.get for the labels" + ); + assert.ok( + !calls.some((call) => call.includes("/actions/")), + "no workflow runs or jobs" + ); +}); + +// Fine-grained tokens can't read check runs (1accc56c): GitHub answers 403, +// and from then on this process lists each workflow run's jobs instead. +test("parse lists workflow jobs once check runs are refused", async (t) => { + const calls = fakeGithub( + t, + parseRoutes(() => ({ + status: 403, + body: { message: "Resource not accessible by personal access token" }, + })) + ); + + const first = await gitManager.parse(githubPull()); + const second = await gitManager.parse(githubPull()); + + for (const pull of [first, second]) { + assert.deepEqual( + pull.commitStatuses.map( + (s) => `${s.data.context}=${s.data.state} ${s.data.target_url}` + ), + [`phpunit=success ${job.html_url}`] + ); + } + assert.equal(calls.filter((call) => call.endsWith("/check-runs")).length, 1); +}); From 75f429db443bd0d9493181627533b1dfd5d2189e Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Fri, 2 Oct 2026 16:22:06 -0700 Subject: [PATCH 3/4] Make Refresh all bring the board to GitHub's state From Sep 25 to Sep 30, prod's container wrote into the dev database, most likely because a deploy started it with the dev env file. Prod's own database never saw those webhooks. By Oct 2, 76 PRs opened in the window were missing from the board and 34 that closed then still read open: https://ifixit.slack.com/archives/C6YD8UBS7/p1790970090546029 Refresh all couldn't fix either. The browser sent one refresh per open PR it already showed, so it never found a missing one, and each was a full refresh. For the 303 open PRs the board held on Oct 2 that's about 5,400 GitHub calls at the rates in the previous commit, more than the token's 5,000 an hour. A press took hours, and the refreshes webhooks start waited behind it in the same queue. Now a press is one job on the server, one at a time: 1. List every open PR, and every PR closed within the board's 14 days. 2. Compare with the board, and refetch only the PRs that differ: - missing from the board, or open on it but not on GitHub - a different head, draft state or labels - more comments or reviews on GitHub than on the board - an updated_at newer than anything the board knows - a CR or QA stamp standing from before the head commit - a check the board still shows running 3. Refetch them one at a time behind a pacer that spaces them by the quota left. Webhook refreshes keep their own queue, so they never wait behind a press. Every board hears how far it got. It lists the configured repos plus every repo with a PR on the board. Webhooks come in for the whole org, so the board holds PRs from repos the config doesn't list: 53 of the 76 missing were in ops. Why counts and not only updated_at: comment and review webhooks never write the pull's row, though GitHub's updated_at moves with them. 14 of 101 open iFixit/ifixit PRs have a comment or review as their newest activity. A check against the row alone would refetch those on every press, so the newest comment and review count as known too. And a timestamp can't see a missed comment once a newer one came in, which is how stamps went missing in #502. The counts can. The listing is GraphQL. One call returns 100 PRs with their counts, and its points come from a separate hourly budget from the REST calls webhooks spend. Octokit runs GraphQL calls one at a time, a second apart, in the same lane as a claim's review request. So a press lists one repo at a time, with one query for both lists. The pacer now skips GraphQL's rate-limit headers, which it would have read as REST's. Measured with a personal token on Oct 2, about 23:15Z, listing all 25 repos took 29 GraphQL calls and 33 seconds, for 251 open and 650 closed PRs. 2 open PRs had a check running then. So a press on a board that matches GitHub costs 29 GraphQL calls and 2 refetches. A real press with a 6-PR board and the DB stubbed out refetched none of the 6 and all 42 that were missing, at 8 REST calls each. Note: a check re-run that finishes while webhooks are down, failed to passed, isn't caught; the row's refresh button or the next push fixes it. Catching it would mean listing every open PR's checks, a REST call each, or GraphQL check runs a fine-grained token may not read. Co-Authored-By: Claude Opus 5.5 --- app.js | 31 +++- lib/git-manager.js | 117 +++++++++++- lib/pacer.js | 16 +- lib/pull-manager.js | 7 + lib/refresh-all.js | 254 ++++++++++++++++++++++++++ test/git-manager-refresh.test.js | 116 ++++++++++++ test/pacer.test.js | 20 ++ test/refresh-all.test.js | 303 +++++++++++++++++++++++++++++++ 8 files changed, 852 insertions(+), 12 deletions(-) create mode 100644 lib/refresh-all.js create mode 100644 test/refresh-all.test.js diff --git a/app.js b/app.js index beb745fd..c5a34a39 100644 --- a/app.js +++ b/app.js @@ -6,9 +6,11 @@ import authManager from './lib/authentication.js'; import socketAuthenticator from './lib/socket-auth.js'; import refresh from './lib/refresh.js'; import pullManager from './lib/pull-manager.js'; -import git from './lib/git-manager.js'; +import git, { observeRateLimit } from './lib/git-manager.js'; import dbManager from './lib/db-manager.js'; import pullQueue from './lib/pull-queue.js'; +import { createRefreshAll } from './lib/refresh-all.js'; +import { createPacer } from './lib/pacer.js'; import mainController from './controllers/main.js'; import hooksController from './controllers/githubHooks.js'; import statsController from './controllers/stats.js'; @@ -24,6 +26,21 @@ import { dirname } from 'path'; const reqLogger = Debug('pulldasher:server:request'); const debug = Debug('pulldasher'); +// "Refresh all" compares GitHub with the board, so it waits for the board to +// load. Its refetches wait on their own pacer, which watches the quota every +// response reports, webhook refreshes included, so a press yields to them. +let boardLoaded; +const refreshAllPacer = createPacer(); +observeRateLimit(refreshAllPacer); +const refreshAll = createRefreshAll({ + board: pullManager.getPulls, + repos: config.repos, + closedSince: pullManager.getOldestAllowedPullTimestamp, + pacer: refreshAllPacer, + ready: new Promise(resolve => (boardLoaded = resolve)), + onProgress: progress => pullManager.broadcast('refreshAllProgress', progress), +}); + const app = express(); const httpServer = createServer(app); const maxPostSize = 1024 * 1024; @@ -92,6 +109,7 @@ dbManager pullManager.updatePull(pull); }); pullQueue.resume(); + boardLoaded(); }) .then(function () { debug('Refreshing all open pulls from the API'); @@ -130,12 +148,23 @@ io.on('connection', function (socket) { socket.user = user; socket.emit('authenticated'); pullManager.addSocket(socket); + // a board that connects mid-press shows how far it got + const progress = refreshAll.progress(); + if (progress && progress.state !== 'done') { + socket.emit('refreshAllProgress', progress); + } } else { socket.emit('unauthenticated'); socket.disconnect(); } }); + socket.on('refreshAll', function () { + if (socket.user) { + refreshAll.start(); + } + }); + socket.on('refresh', function (repo, number) { refresh.pull(repo, number).catch(function (err) { console.error( diff --git a/lib/git-manager.js b/lib/git-manager.js index 534aae97..8b83a54e 100644 --- a/lib/git-manager.js +++ b/lib/git-manager.js @@ -73,13 +73,13 @@ github.hook.error('request', (error, options) => { * Feed a pacer the quota headers from every GitHub response, so it can pace bulk * work against real consumption — crucially the per-pull/issue fan-out deep in * `parse`/`parseIssue`, which is the bulk of a backfill's spend and which the - * caller can't see to observe itself. Only the CLI backfill bins call this (once - * at startup, with their pacer); the server never installs an observer, so its - * webhook/socket responses aren't tracked and nothing is paced. + * caller can't see to observe itself. The CLI backfill bins call this once at + * startup with their pacer, and the server with the pacer "Refresh all" waits + * on; the webhook/socket responses it observes are never paced themselves. * * Installs the hooks at most once per process: they close over the first * pacer, and a second call would only stack duplicate observers on the shared - * client (double-counting every response). The bins call this exactly once. + * client (double-counting every response). Each entry point calls this once. */ let rateLimitObserved = false; export function observeRateLimit(pacer) { @@ -181,6 +181,67 @@ export default { ); }, + /** + * What "Refresh all" compares with the board, for every open pull in + * `repo` and every pull there updated since `closedSince` (a Date) and + * closed: number, state, updatedAt and, for open pulls, the head, draft, + * labels, comment and review counts. GraphQL, because one call returns + * 100 pulls with their counts, and its points come from a separate hourly + * budget from the REST calls the webhooks spend. + */ + getPullsToCompare: async function (repo, closedSince) { + const [owner, name] = parseRepo(repo); + const open = { nodes: [], more: true, cursor: null }; + const closed = { nodes: [], more: true, cursor: null }; + // the first call reads both lists; later ones only the one with more + while (open.more || closed.more) { + const { repository } = await github.graphql(PULLS_TO_COMPARE_QUERY, { + owner, + name, + withOpen: open.more, + openCursor: open.cursor, + withClosed: closed.more, + closedCursor: closed.cursor, + }); + if (open.more) { + takePage(open, repository.open, () => true); + } + if (closed.more) { + takePage( + closed, + repository.closed, + node => Date.parse(node.updatedAt) >= closedSince.getTime() + ); + } + } + return open.nodes + .map(node => { + const head = node.commits.nodes[0]; + return { + repo, + number: node.number, + state: 'open', + updatedAt: node.updatedAt, + draft: node.isDraft, + headSha: node.headRefOid, + headCommittedAt: + head && head.commit.oid === node.headRefOid ? head.commit.committedDate : null, + labels: node.labels.nodes.map(label => label.name), + comments: node.comments.totalCount, + reviews: node.reviews.totalCount, + }; + }) + .concat( + closed.nodes.map(node => ({ + repo, + number: node.number, + state: 'closed', + updatedAt: node.updatedAt, + closedAt: node.closedAt, + })) + ); + }, + /** * Best-effort: request `login` as a reviewer on a pull. Fires as the bot * token — pulldasher keeps no per-user GitHub token — so GitHub records the @@ -892,3 +953,51 @@ function hasNextPage(response) { return Boolean(link) && link.includes('rel="next"'); } +// Octokit's throttling runs GraphQL calls one at a time, a second apart, in +// the lane REST writes (a claim's review request) also wait in, so one query +// reads both lists and a press asks for one repo at a time. +const PULLS_TO_COMPARE_QUERY = `query( + $owner: String! + $name: String! + $withOpen: Boolean! + $openCursor: String + $withClosed: Boolean! + $closedCursor: String +) { + repository(owner: $owner, name: $name) { + open: pullRequests(states: OPEN, first: 100, after: $openCursor) + @include(if: $withOpen) { + pageInfo { hasNextPage endCursor } + nodes { + number + updatedAt + isDraft + headRefOid + labels(first: 100) { nodes { name } } + comments { totalCount } + reviews { totalCount } + commits(last: 1) { nodes { commit { oid committedDate } } } + } + } + closed: pullRequests( + states: [CLOSED, MERGED] + first: 100 + after: $closedCursor + orderBy: { field: UPDATED_AT, direction: DESC } + ) @include(if: $withClosed) { + pageInfo { hasNextPage endCursor } + nodes { number updatedAt closedAt } + } + } +}`; + +/** + * Add a page of a pullRequests connection to `list` while `keep` accepts its + * nodes, and note whether another page is worth reading. + */ +function takePage(list, page, keep) { + const kept = page.nodes.filter(keep); + list.nodes.push(...kept); + list.more = page.pageInfo.hasNextPage && kept.length === page.nodes.length; + list.cursor = page.pageInfo.endCursor; +} diff --git a/lib/pacer.js b/lib/pacer.js index 9cbfb61c..b29d2e5d 100644 --- a/lib/pacer.js +++ b/lib/pacer.js @@ -11,12 +11,12 @@ const LOG_DELAY_THRESHOLD_MS = 5000; /** * A pacer proactively paces bulk GitHub requests so a backfill yields to live * traffic instead of draining the shared token's quota to zero. It is a - * per-process instance the entry point constructs and installs: only the CLI - * backfill bins build a real one (`createPacer`); the server installs nothing - * and runs against `noopPacer`, so live webhook/socket refreshes are never - * delayed. Rate state is global to the token, so within a process every bulk - * call gates against the same view, fed by `observe` on every response (live - * calls included). + * per-process instance the entry point constructs and installs: the CLI + * backfill bins build a real one (`createPacer`), and so does the server for + * "Refresh all". Webhook/socket refreshes run against `noopPacer`, so live + * traffic is never delayed. Rate state is global to the token, so within a + * process every bulk call gates against the same view, fed by `observe` on + * every response (live calls included). * * `observe(headers)` — record the latest quota from any response (free). * `gate()` — await this before each *bulk* unit of work (a consumer drain or a @@ -38,7 +38,9 @@ export function createPacer({ reserve = config.github.bulkReserve ?? 1000 } = {} return { observe: function (headers) { - if (!headers) { + // GraphQL and search answer with their own budgets in the same headers; + // only REST's (`core`) is the quota bulk work spends. + if (!headers || (headers["x-ratelimit-resource"] ?? "core") !== "core") { return; } if (headers["x-ratelimit-remaining"] !== undefined) { diff --git a/lib/pull-manager.js b/lib/pull-manager.js index 7a35db7d..7d7bb8a9 100644 --- a/lib/pull-manager.js +++ b/lib/pull-manager.js @@ -45,6 +45,13 @@ const pullManager = { notifyAboutPullStateChange(pull); }, + + /** Send `event` to every connected board. */ + broadcast: function (event, payload) { + sockets.forEach(function (socket) { + socket.emit(event, payload); + }); + }, }; function sendInitialData(socket) { diff --git a/lib/refresh-all.js b/lib/refresh-all.js new file mode 100644 index 00000000..587f6707 --- /dev/null +++ b/lib/refresh-all.js @@ -0,0 +1,254 @@ +import gitManager from './git-manager.js'; +import dbManager from './db-manager.js'; +import debug from './debug.js'; +import { noopPacer } from './pacer.js'; +import { findMissingOpenPulls, findStaleOpenPulls, processPullItem } from './refresh.js'; + +const log = debug('pulldasher:refresh-all'); + +/** + * "Refresh all": one press brings the board to GitHub's state. It lists the + * open and recently closed pulls of every repo the board shows, compares them + * with the board, and refetches only the pulls that differ, one at a time. + * Each refetch waits for `pacer`, and webhook refreshes run on their own + * queue, so they never wait behind a press. One press runs at a time. + * + * `board()` returns the board's pulls (pull-manager), `repos` the configured + * repos, `closedSince()` the oldest close the board shows. `ready` resolves + * once the board has loaded from the DB. `onProgress` hears every change. + */ +export function createRefreshAll({ + board, + repos, + closedSince, + pacer = noopPacer, + ready = Promise.resolve(), + onProgress = () => {}, +}) { + let progress = null; + + function report(next) { + progress = next; + onProgress(next); + } + + async function run() { + await ready; + const since = closedSince(); + const listed = []; + const listedRepos = []; + let skipped = 0; + // one repo at a time: see PULLS_TO_COMPARE_QUERY in git-manager + for (const repo of coveredRepos(repos, board())) { + try { + listed.push(...(await gitManager.getPullsToCompare(repo, since))); + listedRepos.push(repo); + } catch (err) { + skipped++; + console.error( + 'Refresh all: failed to list the pulls in %s: %s', + repo, + (err && err.message) || err + ); + } + } + const due = pullsToRefresh(board(), listed, listedRepos, since); + log('%s of %s listed pulls differ from the board', due.length, listed.length); + let done = 0; + let failed = 0; + report({ state: 'refreshing', done, total: due.length, failed, skipped }); + for (const { repo, number, why } of due) { + await pacer.gate(); + log('refreshing %s#%s (%s)', repo, number, why); + if (!(await refreshPull(repo, number))) { + failed++; + } + done++; + report({ state: 'refreshing', done, total: due.length, failed, skipped }); + } + report({ state: 'done', done, total: due.length, failed, skipped }); + } + + return { + progress: () => progress, + + /** + * Start a press unless one is running. Resolves when it finishes, or + * returns null when one was already running. + */ + start() { + if (progress && progress.state !== 'done') { + return null; + } + report({ state: 'checking', done: 0, total: 0, failed: 0, skipped: 0 }); + return run().catch(err => { + // a bug, not GitHub: count what it didn't get to as failed, so the + // button frees up and the board says to look at the log + console.error('Refresh all stopped: %s', (err && err.message) || err); + const unfinished = Math.max(1, progress.total - progress.done); + report({ ...progress, state: 'done', failed: progress.failed + unfinished }); + }); + }, + }; +} + +/** Fetch, parse and save one pull. Resolves to whether it saved. */ +async function refreshPull(repo, number) { + let saved = true; + try { + const response = await gitManager.getPull(repo, number); + await new Promise(next => + processPullItem(response, next, { + parse: gitManager.parse, + updateAllPullData: dbManager.updateAllPullData, + onFailure: () => (saved = false), + }) + ); + } catch (err) { + console.error( + 'Refresh all: failed to fetch pull %s in repo %s: %s', + number, + repo, + (err && err.message) || err + ); + saved = false; + } + return saved; +} + +/** + * The configured repos plus every repo with a pull on the board, once each. + * Webhooks arrive for the whole org, so the board holds pulls from repos the + * config doesn't list. + */ +export function coveredRepos(repos, boardPulls) { + const byName = new Map(); + for (const name of repos.map(repo => repo.name).concat(boardPulls.map(pull => pull.data.repo))) { + if (!byName.has(name.toLowerCase())) { + byName.set(name.toLowerCase(), name); + } + } + return [...byName.values()]; +} + +/** + * The pulls where the board and GitHub disagree, each once, with the first + * reason found. `listed` is getPullsToCompare's output for `listedRepos`, the + * repos that answered; `closedSince` is the oldest close the board shows. + */ +export function pullsToRefresh(boardPulls, listed, listedRepos, closedSince) { + const key = (repo, number) => repo.toLowerCase() + '#' + number; + const onBoard = new Map(boardPulls.map(pull => [key(pull.data.repo, pull.data.number), pull])); + const boardOpen = boardPulls + .filter(pull => pull.isOpen()) + .map(pull => ({ repo: pull.data.repo, number: pull.data.number })); + // shaped like pulls.list items, which the reconcile helpers read + const listedOpen = listed + .filter(pull => pull.state === 'open') + .map(pull => ({ number: pull.number, base: { repo: { full_name: pull.repo } } })); + const due = new Map(); + const add = (repo, number, why) => { + if (!due.has(key(repo, number))) { + due.set(key(repo, number), { repo, number, why }); + } + }; + + // open on GitHub but not on the board: a lost `opened` or `reopened` + for (const pull of findMissingOpenPulls(boardOpen, listedOpen)) { + add(pull.base.repo.full_name, pull.number, 'missing'); + } + // open on the board but not on GitHub: a lost `closed` + for (const pull of findStaleOpenPulls(boardOpen, listedOpen, listedRepos)) { + add(pull.repo, pull.number, 'closed'); + } + for (const pull of listed) { + const shown = onBoard.get(key(pull.repo, pull.number)); + if (pull.state === 'closed') { + // closed recently enough to be on the board, and missing from it: + // opened and closed while webhooks weren't arriving + if (!shown && Date.parse(pull.closedAt) >= closedSince.getTime()) { + add(pull.repo, pull.number, 'missing'); + } + } else if (shown && shown.isOpen()) { + const why = difference(shown, pull); + if (why) { + add(pull.repo, pull.number, why); + } + } + } + return [...due.values()]; +} + +/** + * The first way the board's copy of an open pull differs from GitHub's + * listing of it, or null when they agree. + */ +export function difference(pull, listed) { + if (pull.data.head.sha !== listed.headSha) { + return 'head'; + } + if (Boolean(pull.data.draft) !== listed.draft) { + return 'draft'; + } + const labels = pull.labels.map(label => label.data.title); + if (labels.length !== listed.labels.length || !labels.every(l => listed.labels.includes(l))) { + return 'labels'; + } + // Counts catch a lost comment or review even after newer ones arrived, + // which a timestamp can't. The DB keeps deleted comments, so only more on + // GitHub counts as a difference. + if (listed.comments > pull.comments.filter(c => c.data.comment_type === 'issue').length) { + return 'comments'; + } + if (listed.reviews > pull.reviews.length) { + return 'reviews'; + } + if (Date.parse(listed.updatedAt) > newestKnown(pull)) { + return 'updated'; + } + if (listed.headCommittedAt && lapsedStampStands(pull, Date.parse(listed.headCommittedAt))) { + return 'stamp'; + } + // a check's result never moves updated_at, so a lost one shows only as a + // check the board still holds as running + if (pull.commitStatuses.some(status => status.data.state === 'pending')) { + return 'ci'; + } + return null; +} + +/** + * The newest time the board knows for a pull. GitHub's updated_at moves with + * every comment and review, but those webhooks don't touch the pull's row, so + * comparing with the row alone would refetch every pull whose newest activity + * is a comment. + */ +function newestKnown(pull) { + const times = [pull.data.updated_at] + .concat(pull.comments.map(comment => comment.data.created_at)) + .concat(pull.reviews.map(review => review.data.submitted_at)) + .map(time => (time ? new Date(time).getTime() : NaN)) + .filter(Number.isFinite); + return times.length ? Math.max(...times) : 0; +} + +/** + * A CR or QA stamp the board shows standing though it predates the head + * commit, which a refresh would lapse: a lost push, hidden because a later + * webhook already wrote the new head. GitHub approvals are exempt, as in + * git-manager's parse. + */ +function lapsedStampStands(pull, headCommittedAt) { + const approvals = new Set( + pull.reviews + .filter(review => String(review.data.state).toUpperCase() === 'APPROVED') + .map(review => review.data.review_id) + ); + return pull.signatures.some( + sig => + (sig.data.type === 'CR' || sig.data.type === 'QA') && + sig.data.active === 1 && + !approvals.has(sig.data.comment_id) && + sig.data.created_at < headCommittedAt + ); +} diff --git a/test/git-manager-refresh.test.js b/test/git-manager-refresh.test.js index b4e55859..c6fb3731 100644 --- a/test/git-manager-refresh.test.js +++ b/test/git-manager-refresh.test.js @@ -151,3 +151,119 @@ test("parse lists workflow jobs once check runs are refused", async (t) => { } assert.equal(calls.filter((call) => call.endsWith("/check-runs")).length, 1); }); + +test("getPullsToCompare reads both lists in one call, then only the one with more", async (t) => { + const node = (number, extra = {}) => ({ + number, + updatedAt: "2026-09-28T00:00:00Z", + isDraft: false, + headRefOid: `sha${number}`, + labels: { nodes: [{ name: "size: S" }] }, + comments: { totalCount: 2 }, + reviews: { totalCount: 1 }, + commits: { + nodes: [ + { + commit: { + oid: `sha${number}`, + committedDate: "2026-09-27T00:00:00Z", + }, + }, + ], + }, + ...extra, + }); + const connection = (nodes, endCursor) => ({ + pageInfo: { hasNextPage: Boolean(endCursor), endCursor }, + nodes, + }); + const asked = []; + fakeGithub(t, [ + [ + /^\/graphql$/, + ({ variables }) => { + asked.push( + `open ${variables.withOpen} ${variables.openCursor}, closed ${variables.withClosed}` + ); + const repository = {}; + if (variables.withOpen) { + repository.open = variables.openCursor + ? connection([ + node(2, { + commits: { + nodes: [{ commit: { oid: "old", committedDate: "x" } }], + }, + }), + ]) + : connection([node(1)], "c1"); + } + if (variables.withClosed) { + // newest update first: #4 is older than the cutoff, so page 2 (empty + // here) is never asked for + repository.closed = variables.closedCursor + ? connection([]) + : connection( + [ + { + number: 3, + updatedAt: "2026-09-27T00:00:00Z", + closedAt: "2026-09-26T00:00:00Z", + }, + { + number: 4, + updatedAt: "2026-09-01T00:00:00Z", + closedAt: "2026-09-01T00:00:00Z", + }, + ], + "c2" + ); + } + return { body: { data: { repository } } }; + }, + ], + ]); + + const pulls = await gitManager.getPullsToCompare( + "test/repo-a", + new Date("2026-09-18T00:00:00Z") + ); + + assert.deepEqual(asked, [ + "open true null, closed true", + "open true c1, closed false", + ]); + assert.deepEqual(pulls, [ + { + repo: "test/repo-a", + number: 1, + state: "open", + updatedAt: "2026-09-28T00:00:00Z", + draft: false, + headSha: "sha1", + headCommittedAt: "2026-09-27T00:00:00Z", + labels: ["size: S"], + comments: 2, + reviews: 1, + }, + { + repo: "test/repo-a", + number: 2, + state: "open", + updatedAt: "2026-09-28T00:00:00Z", + draft: false, + headSha: "sha2", + // the last commit listed isn't the head, so its date says nothing + headCommittedAt: null, + labels: ["size: S"], + comments: 2, + reviews: 1, + }, + { + repo: "test/repo-a", + number: 3, + state: "closed", + updatedAt: "2026-09-27T00:00:00Z", + closedAt: "2026-09-26T00:00:00Z", + }, + ]); +}); diff --git a/test/pacer.test.js b/test/pacer.test.js index 6077151e..003da950 100644 --- a/test/pacer.test.js +++ b/test/pacer.test.js @@ -129,3 +129,23 @@ test("createPacer spaces consecutive gates by observed consumption", async () => "second gate should pace out the request spent since the last gate" ); }); + +// GraphQL and search answer with their own budgets in the same headers. A +// pacer that read them as REST's would pause bulk work for a quota it isn't +// spending, here until a reset a few seconds out (short, so a regression fails +// instead of holding the run open). +test("createPacer ignores the GraphQL budget's headers", async () => { + const pacer = createPacer({ reserve: 10 }); + pacer.observe({ + "x-ratelimit-resource": "graphql", + "x-ratelimit-remaining": "0", + "x-ratelimit-reset": String(Math.ceil(Date.now() / 1000) + 3), + "x-ratelimit-used": "5000", + }); + + const paused = await Promise.race([ + pacer.gate().then(() => false), + new Promise((resolve) => setTimeout(() => resolve(true), 1000)), + ]); + assert.equal(paused, false); +}); diff --git a/test/refresh-all.test.js b/test/refresh-all.test.js new file mode 100644 index 00000000..a584b4ad --- /dev/null +++ b/test/refresh-all.test.js @@ -0,0 +1,303 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import gitManager from "../lib/git-manager.js"; +import dbManager from "../lib/db-manager.js"; +import Pull from "../models/pull.js"; +import { + coveredRepos, + createRefreshAll, + pullsToRefresh, +} from "../lib/refresh-all.js"; + +const CLOSED_SINCE = new Date("2026-09-18T00:00:00Z"); +const HEAD_COMMITTED = "2026-09-20T10:00:00Z"; + +// A pull as the board holds it (pull-manager keeps Pull objects loaded from +// the DB), in step with listed() below unless a test changes one side. +function shown({ + repo = "test/repo-a", + number = 1, + state = "open", + sha = "head1", + draft = false, + updatedAt = "2026-09-20T12:00:00Z", + labels = ["size: S"], + comments = ["2026-09-20T11:00:00Z"], + reviews = [], + signatures = [], + statuses = ["success"], +} = {}) { + const user = { login: "someone" }; + return new Pull( + { + repo, + number, + state, + draft, + updated_at: new Date(updatedAt), + head: { sha }, + user: { login: "author" }, + cr_req: 1, + qa_req: 1, + }, + signatures.map((sig) => ({ + data: { user, active: 1, ...sig, created_at: new Date(sig.at) }, + })), + comments.map((at, i) => ({ + data: { + comment_type: "issue", + comment_id: i, + created_at: new Date(at), + user, + }, + })), + reviews.map((review) => ({ + data: { ...review, submitted_at: new Date(review.at), user }, + })), + statuses.map((state) => ({ data: { state } })), + labels.map((title) => ({ data: { title } })) + ); +} + +// The same pull as getPullsToCompare lists it. +function listed({ + repo = "test/repo-a", + number = 1, + sha = "head1", + draft = false, + updatedAt = "2026-09-20T12:00:00Z", + labels = ["size: S"], + comments = 1, + reviews = 0, +} = {}) { + return { + repo, + number, + state: "open", + updatedAt, + draft, + headSha: sha, + headCommittedAt: HEAD_COMMITTED, + labels, + comments, + reviews, + }; +} + +const closedListing = (number, closedAt, repo = "test/repo-a") => ({ + repo, + number, + state: "closed", + updatedAt: closedAt, + closedAt, +}); + +function due(board, listing, listedRepos = ["test/repo-a"]) { + return pullsToRefresh(board, listing, listedRepos, CLOSED_SINCE).map( + (pull) => `${pull.repo}#${pull.number} ${pull.why}` + ); +} + +test("a pull that matches GitHub isn't refreshed", () => { + assert.deepEqual(due([shown()], [listed()]), []); +}); + +test("a pull open on GitHub but missing from the board is refreshed", () => { + assert.deepEqual(due([], [listed({ number: 7 })]), ["test/repo-a#7 missing"]); +}); + +test("a pull the board holds closed that GitHub lists open is refreshed", () => { + assert.deepEqual(due([shown({ state: "closed" })], [listed()]), [ + "test/repo-a#1 missing", + ]); +}); + +test("a pull the board holds open that GitHub doesn't list open is refreshed", () => { + const board = [ + shown({ number: 2 }), + shown({ repo: "test/repo-b", number: 3 }), + ]; + + // repo-b's listing failed, which proves nothing about its pulls + assert.deepEqual(due(board, []), ["test/repo-a#2 closed"]); +}); + +test("a pull closed since the cutoff and missing from the board is refreshed", () => { + const listing = [ + closedListing(8, "2026-09-27T10:00:00Z"), + // closed before the board's cutoff: the board wouldn't show it + closedListing(9, "2026-09-01T10:00:00Z"), + ]; + + assert.deepEqual(due([], listing), ["test/repo-a#8 missing"]); + // already on the board: nothing to add + assert.deepEqual( + due([shown({ number: 8, state: "closed" })], listing.slice(0, 1)), + [] + ); +}); + +test("a pull pushed to since the board read it is refreshed", () => { + assert.deepEqual(due([shown()], [listed({ sha: "head2" })]), [ + "test/repo-a#1 head", + ]); +}); + +test("a pull marked ready for review since the board read it is refreshed", () => { + assert.deepEqual(due([shown({ draft: true })], [listed()]), [ + "test/repo-a#1 draft", + ]); +}); + +test("a pull labeled since the board read it is refreshed", () => { + assert.deepEqual( + due([shown()], [listed({ labels: ["size: S", "project:x"] })]), + ["test/repo-a#1 labels"] + ); +}); + +test("a pull with a comment the board missed is refreshed, even after newer ones arrived", () => { + const board = [ + shown({ comments: ["2026-09-20T11:00:00Z", "2026-09-21T09:00:00Z"] }), + ]; + + assert.deepEqual( + due(board, [listed({ comments: 3, updatedAt: "2026-09-21T09:00:00Z" })]), + ["test/repo-a#1 comments"] + ); + // the DB keeps deleted comments, so more on the board than on GitHub is fine + assert.deepEqual( + due(board, [listed({ comments: 1, updatedAt: "2026-09-21T09:00:00Z" })]), + [] + ); +}); + +test("a pull with a review the board missed is refreshed", () => { + assert.deepEqual(due([shown()], [listed({ reviews: 1 })]), [ + "test/repo-a#1 reviews", + ]); +}); + +test("a pull GitHub updated after everything the board knows is refreshed", () => { + assert.deepEqual( + due([shown()], [listed({ updatedAt: "2026-09-21T00:00:00Z" })]), + ["test/repo-a#1 updated"] + ); +}); + +// Comment and review webhooks never touch the pull's row, but GitHub's +// updated_at moves with them: the newest comment counts as known. +test("a pull whose newest activity is a comment the board has isn't refreshed", () => { + const board = [shown({ comments: ["2026-09-22T08:00:00Z"] })]; + + assert.deepEqual( + due(board, [listed({ updatedAt: "2026-09-22T08:00:00Z" })]), + [] + ); +}); + +test("a stamp the board shows standing though it predates the head commit is refreshed", () => { + const stamp = { type: "CR", comment_id: 50, at: "2026-09-20T09:00:00Z" }; + + assert.deepEqual(due([shown({ signatures: [stamp] })], [listed()]), [ + "test/repo-a#1 stamp", + ]); + // an approval GitHub kept across the push stays standing, as parse() keeps it + const reviews = [ + { review_id: 50, state: "APPROVED", at: "2026-09-20T09:00:00Z" }, + ]; + assert.deepEqual( + due([shown({ signatures: [stamp], reviews })], [listed({ reviews: 1 })]), + [] + ); +}); + +test("a pull whose checks the board still shows running is refreshed", () => { + assert.deepEqual( + due([shown({ statuses: ["success", "pending"] })], [listed()]), + ["test/repo-a#1 ci"] + ); +}); + +test("coveredRepos adds every repo the board shows to the configured ones", () => { + const board = [ + shown({ repo: "iFixit/ops" }), + shown({ repo: "ifixit/ifixit" }), + ]; + + assert.deepEqual(coveredRepos([{ name: "iFixit/ifixit" }], board), [ + "iFixit/ifixit", + "iFixit/ops", + ]); +}); + +// A press lists the board's repos, refetches only what differs, one pull per +// pacer slot, and tells every board how far it got. +test("a press refreshes only the pulls that differ and reports its progress", async (t) => { + const board = [ + shown({ number: 1 }), + shown({ repo: "iFixit/ops", number: 2 }), + ]; + const listedRepos = []; + t.mock.method(gitManager, "getPullsToCompare", (repo) => { + listedRepos.push(repo); + return Promise.resolve( + repo === "test/repo-a" + ? [listed({ number: 1 }), listed({ number: 5 })] + : [] + ); + }); + const fetched = []; + t.mock.method(gitManager, "getPull", (repo, number) => { + fetched.push(`${repo}#${number}`); + if (number === 2) return Promise.reject(new Error("transient 502")); + return Promise.resolve({ number, base: { repo: { full_name: repo } } }); + }); + t.mock.method(gitManager, "parse", (response) => Promise.resolve(response)); + t.mock.method(dbManager, "updateAllPullData", () => Promise.resolve()); + t.mock.method(console, "error", () => {}); + let gates = 0; + const progress = []; + const press = createRefreshAll({ + board: () => board, + repos: [{ name: "test/repo-a" }], + closedSince: () => CLOSED_SINCE, + pacer: { gate: () => Promise.resolve(gates++) }, + onProgress: (p) => + progress.push(`${p.state} ${p.done}/${p.total} failed ${p.failed}`), + }); + + await press.start(); + + assert.deepEqual(listedRepos, ["test/repo-a", "iFixit/ops"]); + assert.deepEqual(fetched, ["test/repo-a#5", "iFixit/ops#2"]); + assert.equal(gates, 2); + assert.deepEqual(progress, [ + "checking 0/0 failed 0", + "refreshing 0/2 failed 0", + "refreshing 1/2 failed 0", + "refreshing 2/2 failed 1", + "done 2/2 failed 1", + ]); +}); + +test("a second press while one runs doesn't start another", async (t) => { + let listings = 0; + t.mock.method(gitManager, "getPullsToCompare", () => { + listings++; + return Promise.resolve([]); + }); + const press = createRefreshAll({ + board: () => [], + repos: [{ name: "test/repo-a" }], + closedSince: () => CLOSED_SINCE, + }); + + const first = press.start(); + assert.equal(press.start(), null); + await first; + await press.start(); + + assert.equal(listings, 2); + assert.equal(press.progress().state, "done"); +}); From ba396be95d83edc1b1e63931f4aab3762f18ffea Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Fri, 2 Oct 2026 16:22:18 -0700 Subject: [PATCH 4/4] Show a press's progress on every board MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit moved Refresh all to the server. The button now sends one refreshAll event instead of a refresh per open PR. The board takes the press's progress from the server, where it used to count the pullChange events that came back. Every open board shows the same press, and the button waits while one runs. The status line says what the press is doing: "checking GitHub…", then "refreshing 3 of 12…". Then it says what the press did, like "refreshed 11 · 1 failed", or "up to date" when nothing differed. It stays 6 seconds, then clears. The header shows the same words, from one function. The dummy board plays a press of 4 PRs with one failing, so every state shows without GitHub. Co-Authored-By: Claude Opus 5.5 --- frontend-v2/src/app.tsx | 6 +- frontend-v2/src/backend/socket.ts | 66 +++++++++++++--- frontend-v2/src/components/Settings.tsx | 27 +++---- frontend-v2/src/store.test.ts | 24 +++++- frontend-v2/src/store.ts | 101 +++++++++--------------- shared/types.ts | 12 +++ 6 files changed, 141 insertions(+), 95 deletions(-) diff --git a/frontend-v2/src/app.tsx b/frontend-v2/src/app.tsx index 3f3f463c..575b9344 100644 --- a/frontend-v2/src/app.tsx +++ b/frontend-v2/src/app.tsx @@ -18,7 +18,7 @@ import { buildReviewerPools, turnFor } from './model/rotation'; import { shipRelevance, shippedToast } from './model/shipped'; import { notificationStale } from './model/notificationRelevance'; import type { Toast } from './model/toast'; -import { claimReview, isSnoozed, usePulldasher } from './store'; +import { claimReview, isSnoozed, refreshAllText, usePulldasher } from './store'; import { primeScope, useScope } from './prefs'; import { useBoardHotkeys, useHeaderHeightVar } from './hooks'; import { ageRotDays, getSettings, type Settings as SettingsShape, useSettings } from './settings'; @@ -961,9 +961,7 @@ export function App() { // Out of flow (anchored left of the cluster): transient text // must not shove the bell/legend/cog sideways mid-aim. - {refreshProgress.done === refreshProgress.total - ? `refreshed ${refreshProgress.total}` - : `refreshing ${refreshProgress.done} of ${refreshProgress.total}`} + {refreshAllText(refreshProgress)} )} void) => () => void; refreshPull: (repo: string, number: number) => void; + /** Ask the server to bring the board to GitHub's state. Every board hears + * how far it got through onRefreshAll. */ + refreshAll: () => void; + onRefreshAll: (handler: (progress: RefreshAllProgress) => void) => () => void; /** Ask to review a pull. The server derives the login from the socket's * own auth — no login argument here. A claim is a GitHub review request * the reviewer made on themselves (see types.ts's review_requests), so it @@ -167,6 +176,16 @@ function liveBackend(): Backend { refreshPull(repo, number) { getSocket().emit('refresh', repo, number); }, + refreshAll() { + getSocket().emit('refreshAll'); + }, + onRefreshAll(handler) { + const s = getSocket(); + s.on('refreshAllProgress', handler); + return () => { + s.off('refreshAllProgress', handler); + }; + }, claimReview(repo, number) { getSocket().emit('claimReview', repo, number); }, @@ -179,7 +198,8 @@ function liveBackend(): Backend { function dummyBackend(): Backend { let emit: ((payload: InitializePayload | PullData) => void) | null = null; let loaded: InitializePayload | null = null; - let staggered = 0; + let onPress: ((progress: RefreshAllProgress) => void) | null = null; + let pressing = false; /** Mutate one dummy pull in place and re-emit it through the normal * pullChange path — the dummy board's stand-in for a server round trip @@ -232,20 +252,46 @@ function dummyBackend(): Backend { return () => undefined; }, refreshPull(repo, number) { - // The dummy data is static, but the refresh-progress UI still needs - // arrivals to count — re-emit the same pull on a stagger so the - // "X of N" counter animates to completion like a real refresh would. + // The dummy data is static: re-emit the same pull, as a server would + // after refetching it. const pull = loaded?.pulls.find(p => p.repo === repo && p.number === number); - if (!pull || !emit) return; - staggered += 1; + if (pull) setTimeout(() => emit?.(pull), 300); + }, + refreshAll() { + // No GitHub behind the dummy board: play a press that finds 4 pulls + // to refetch, one failing, so every state of the status line shows. + if (pressing) return; + pressing = true; + const total = 4; + const report = (p: RefreshAllProgress) => onPress?.(p); + report({ state: 'checking', done: 0, total: 0, failed: 0, skipped: 0 }); + for (let done = 0; done <= total; done++) { + setTimeout( + () => + report({ + state: 'refreshing', + done, + total, + failed: done === total ? 1 : 0, + skipped: 0, + }), + 1200 + done * 500 + ); + } setTimeout( () => { - staggered -= 1; - emit?.(pull); + pressing = false; + report({ state: 'done', done: total, total, failed: 1, skipped: 0 }); }, - 300 + staggered * 15 + 1300 + total * 500 ); }, + onRefreshAll(handler) { + onPress = handler; + return () => { + onPress = null; + }; + }, claimReview(repo, number) { updatePull(repo, number, pull => ({ ...pull, diff --git a/frontend-v2/src/components/Settings.tsx b/frontend-v2/src/components/Settings.tsx index 177b0549..1803c3a2 100644 --- a/frontend-v2/src/components/Settings.tsx +++ b/frontend-v2/src/components/Settings.tsx @@ -2,7 +2,7 @@ import { useEffect, useRef, useState } from 'react'; import { createPortal } from 'react-dom'; import { Settings as SettingsIcon, Trash2, X } from 'lucide-react'; import { clearStoredPrefs } from '../storage'; -import { refreshAll, usePulldasher } from '../store'; +import { refreshAll, refreshAllText, usePulldasher } from '../store'; import { addCodeRegion, removeCodeRegion, @@ -119,7 +119,6 @@ export function Settings({ const { refreshProgress } = usePulldasher(); const panelRef = useRef(null); const triggerRef = useRef(null); - const [refreshNote, setRefreshNote] = useState(''); const { armed: armReset, run: runReset } = useArmedConfirm(); // open on demand — the code-regions tip's "Set them up" action dispatches @@ -414,21 +413,17 @@ export function Settings({
- Re-fetch every open PR from GitHub now, instead of waiting for the - next webhook. The board updates as each one comes back. + Check the board against GitHub and re-fetch every PR it has wrong, + including ones it missed or still shows open. The board updates as + each one comes back.
{ - // live progress (store.refreshProgress) takes over - // from here; the local note only covers the no-op - if (refreshAll() === 0) { - setRefreshNote('nothing to refresh'); - setTimeout(() => setRefreshNote(''), 2500); - } - }} + // one press at a time, for every board: the server + // ignores a second while one runs + disabled={!!refreshProgress && refreshProgress.state !== 'done'} + onClick={() => refreshAll()} > Refresh all @@ -436,11 +431,7 @@ export function Settings({ when the text lands — a screen reader hears the confirmation, not just sighted users */} - {refreshProgress - ? refreshProgress.done === refreshProgress.total - ? `refreshed ${refreshProgress.total}` - : `refreshing ${refreshProgress.done} of ${refreshProgress.total}…` - : refreshNote} + {refreshProgress ? refreshAllText(refreshProgress) : ''}
diff --git a/frontend-v2/src/store.test.ts b/frontend-v2/src/store.test.ts index 20b6c1a3..d5dad8a6 100644 --- a/frontend-v2/src/store.test.ts +++ b/frontend-v2/src/store.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from 'vitest'; import { pullKey } from '../../shared/format'; import type { PullData } from '../../shared/types'; -import { isSnoozed, type SnoozeRecord } from './store'; +import { isSnoozed, refreshAllText, type SnoozeRecord } from './store'; // isSnoozed reads only repo/number/updated_at and a few status counts, so a // minimal pull is enough; the outer cast keeps the fixture to what it touches. @@ -145,3 +145,25 @@ describe('isSnoozed', () => { ).toBe(false); }); }); + +describe('refreshAllText', () => { + const done = (total: number, failed = 0, skipped = 0) => + refreshAllText({ state: 'done', done: total, total, failed, skipped }); + + it('says how far a press got while it runs', () => { + expect(refreshAllText({ state: 'checking', done: 0, total: 0, failed: 0, skipped: 0 })).toBe( + 'checking GitHub…' + ); + expect( + refreshAllText({ state: 'refreshing', done: 2, total: 5, failed: 0, skipped: 0 }) + ).toBe('refreshing 2 of 5…'); + }); + + it('says what a finished press did, failures included', () => { + expect(done(0)).toBe('up to date'); + expect(done(5)).toBe('refreshed 5'); + expect(done(5, 1)).toBe('refreshed 4 · 1 failed'); + expect(done(1, 1)).toBe('1 failed'); + expect(done(0, 0, 2)).toBe('up to date · couldn’t read 2 repos'); + }); +}); diff --git a/frontend-v2/src/store.ts b/frontend-v2/src/store.ts index 57831b53..b838b544 100644 --- a/frontend-v2/src/store.ts +++ b/frontend-v2/src/store.ts @@ -10,12 +10,12 @@ import { isBotLogin } from '../../shared/model/visibility'; import { getSettings, subscribeSettings } from './settings'; import { epoch, pullKey } from '../../shared/format'; import { readStorage, writeStorage } from './storage'; -import type { PullData, RepoSpec } from '../../shared/types'; +import type { PullData, RefreshAllProgress, RepoSpec } from '../../shared/types'; /** * The one store: raw pulls keyed by repo#number, re-derived and re-published * as a sorted snapshot whenever the socket delivers. Throttled so a burst of - * pullChange events (bulk refresh on the server) renders once, not N times. + * pullChange events (a "Refresh all" on the server) renders once, not N times. */ export interface Snapshot { @@ -38,9 +38,9 @@ export interface Snapshot { * review baseline that wakes it): hidden for a day or until it changes. * Persisted per-browser. */ snoozed: Readonly>; - /** "Refresh all" in progress (or just finished): how many of the pulls - * queued at kickoff have reported back. Null when no refresh is running. */ - refreshProgress: { done: number; total: number } | null; + /** "Refresh all" running on the server, or finished a moment ago. Null + * otherwise. */ + refreshProgress: RefreshAllProgress | null; } const LAST_SEEN_KEY = 'pd2.lastSeen'; @@ -59,34 +59,20 @@ let authFailed = false; let lastPayloadAt = 0; const listeners = new Set<() => void>(); -// "Refresh all" progress: the set of pull keys still awaiting a pullChange -// since the last refreshAll() kickoff. total is fixed at kickoff so the -// header/Settings chip reads "N of TOTAL" even as pending shrinks. Null -// means no refresh is in flight (or its grace window has elapsed). -let refreshTracking: { pending: Set; total: number } | null = null; -let refreshCompleteTimer: ReturnType | null = null; +// "Refresh all" as the server last reported it. A finished press stays on +// screen a few seconds, or a fast one would flash and clear before anyone +// reads it. +let refreshProgress: RefreshAllProgress | null = null; +let refreshClearTimer: ReturnType | null = null; -/** A reconnect resends everything as one 'initialize' payload, which isn't - * the per-pull pullChange this tracking is counting — treat it as "the - * refresh's work is moot", not "N more arrived", or the counter would stall - * short of total forever. */ -function clearRefreshTracking() { - if (refreshCompleteTimer != null) clearTimeout(refreshCompleteTimer); - refreshCompleteTimer = null; - refreshTracking = null; -} - -/** Count a pullChange toward the in-flight refresh, if one is running. On - * the last arrival, hold the finished "N of N" on screen briefly — a fast - * board would otherwise flash the count and clear it before anyone reads it. */ -function noteRefreshArrival(key: string) { - if (!refreshTracking || !refreshTracking.pending.delete(key)) return; - if (refreshTracking.pending.size > 0) return; - refreshCompleteTimer = setTimeout(() => { - refreshTracking = null; - refreshCompleteTimer = null; - schedulePublish(); - }, 4000); +function setRefreshProgress(progress: RefreshAllProgress | null) { + if (refreshClearTimer != null) clearTimeout(refreshClearTimer); + refreshClearTimer = null; + refreshProgress = progress; + if (progress?.state === 'done') { + refreshClearTimer = setTimeout(() => setRefreshProgress(null), 6000); + } + schedulePublish(); } // The marker moves ONLY by the user's hand — the "Recently updated" lane's @@ -278,12 +264,7 @@ function publish() { lastPayloadAt, lastSeen, snoozed: { ...snoozed }, - refreshProgress: refreshTracking - ? { - done: refreshTracking.total - refreshTracking.pending.size, - total: refreshTracking.total, - } - : null, + refreshProgress, }; for (const fn of listeners) fn(); } @@ -324,17 +305,16 @@ function start() { extraBots = new Set(payload.bots ?? []); for (const p of payload.pulls) raw.set(pullKey(p), p); initialized = true; - // a reconnect resends everything; counting it as refresh progress - // would either double-count or strand the tracker short of total - clearRefreshTracking(); + // a press that ended while this board was away mustn't stay stuck + // on screen; the server sends one still running right after this + setRefreshProgress(null); } else { - const key = pullKey(payload); - raw.set(key, payload); - noteRefreshArrival(key); + raw.set(pullKey(payload), payload); } lastPayloadAt = Date.now() / 1000; schedulePublish(); }); + backend.onRefreshAll(setRefreshProgress); backend.onConnection(state => { connection = state; schedulePublish(); @@ -377,23 +357,20 @@ export function releaseReview(pull: Pick): void { } /** - * Settings action: ask the server to re-fetch every open pull from GitHub. - * The socket protocol has no bulk refresh, so this fans out one per-pull - * refresh (the same event a row's refresh button sends) and lets the server's - * serial refresh queue work through them. Returns how many were queued so the - * UI can say so. Closed pulls are historical, so they're left out. + * Settings action: ask the server to bring the board to GitHub's state. It + * compares GitHub with the board and refetches only what differs; every open + * board hears its progress, so this returns nothing. */ -export function refreshAll(): number { - const opens = [...raw.values()].filter(p => p.state === 'open'); - if (opens.length === 0) return 0; - // a re-click mid-refresh restarts the count from this batch, not a merge - // with the last one's leftovers - clearRefreshTracking(); - refreshTracking = { - pending: new Set(opens.map(pullKey)), - total: opens.length, - }; - for (const p of opens) backend.refreshPull(p.repo, p.number); - schedulePublish(); - return opens.length; +export const refreshAll = backend.refreshAll; + +/** One wording for a press, wherever it shows (the header, Settings). */ +export function refreshAllText(p: RefreshAllProgress): string { + if (p.state === 'checking') return 'checking GitHub…'; + if (p.state === 'refreshing') return `refreshing ${p.done} of ${p.total}…`; + const refreshed = p.done - p.failed; + const parts = refreshed > 0 || p.failed > 0 ? [] : ['up to date']; + if (refreshed > 0) parts.push(`refreshed ${refreshed}`); + if (p.failed > 0) parts.push(`${p.failed} failed`); + if (p.skipped > 0) parts.push(`couldn’t read ${p.skipped} repo${p.skipped === 1 ? '' : 's'}`); + return parts.join(' · '); } diff --git a/shared/types.ts b/shared/types.ts index 062542d4..d00d01e8 100644 --- a/shared/types.ts +++ b/shared/types.ts @@ -145,6 +145,18 @@ export interface InitializePayload { weightLabels?: Record; } +/** "Refresh all" on the server, sent to every board: it checks GitHub, then + * refetches the pulls the board has wrong, one press at a time. */ +export interface RefreshAllProgress { + state: 'checking' | 'refreshing' | 'done'; + /** pulls refetched so far, of the `total` that differed from GitHub */ + done: number; + total: number; + /** pulls that couldn't be refetched, and repos GitHub didn't list */ + failed: number; + skipped: number; +} + export interface TokenResponse { socketToken: string; user: string;