Skip to content

Commit efb4b4a

Browse files
centdixclaude
andauthored
feat: rate-limit mitigation for GitHub API calls (#69)
* feat: rate-limit mitigation for GitHub API calls Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: use poll-based activity tracking instead of SSE client count Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address PR review — cache eviction, header fallback, rename isActive Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 39ba997 commit efb4b4a

4 files changed

Lines changed: 144 additions & 18 deletions

File tree

backend/src/notifications.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,21 @@ function broadcast(event: SseEvent): void {
4040
}
4141
}
4242

43+
// --- Dashboard activity tracking (used to pause PR monitor) ---
44+
45+
const ACTIVITY_TIMEOUT_MS = 15_000;
46+
let lastActivityAt = Date.now();
47+
48+
/** Call on every frontend poll to mark dashboard as active. */
49+
export function touchActivity(): void {
50+
lastActivityAt = Date.now();
51+
}
52+
53+
/** Returns true if a dashboard client has polled recently. */
54+
export function hasDashboardActivity(): boolean {
55+
return Date.now() - lastActivityAt < ACTIVITY_TIMEOUT_MS;
56+
}
57+
4358
// --- Public API ---
4459

4560
export function addNotification(

backend/src/pr.ts

Lines changed: 111 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ interface GhPrEntry {
6262
number: number;
6363
headRefName: string;
6464
state: string;
65+
updatedAt: string;
6566
statusCheckRollup: GhCheckEntry[] | null;
6667
url: string;
6768
comments: GhComment[];
@@ -81,6 +82,7 @@ export interface PrEntry {
8182
number: number;
8283
state: "open" | "closed" | "merged";
8384
url: string;
85+
updatedAt: string;
8486
ciStatus: "none" | "pending" | "success" | "failed";
8587
ciChecks: CiCheck[];
8688
comments: PrComment[];
@@ -90,6 +92,17 @@ type FetchPrsResult =
9092
| { ok: true; data: Map<string, PrEntry> }
9193
| { ok: false; error: string };
9294

95+
// ── Caches for rate-limit mitigation ─────────────────────────────────────────
96+
97+
/** Last-seen updatedAt per PR URL — used to skip unchanged PRs' review comments. */
98+
const prUpdatedAtCache = new Map<string, string>();
99+
100+
/** Cached review comments per PR URL — reused when updatedAt hasn't changed. */
101+
const prCommentsCache = new Map<string, PrComment[]>();
102+
103+
/** ETag cache for gh api review comment responses. Keyed by API path. */
104+
const etagCache = new Map<string, { etag: string; comments: PrComment[] }>();
105+
93106
// ── Pure helper functions (exported for unit testing) ─────────────────────────
94107

95108
/** Summarize CI check status from a statusCheckRollup array. */
@@ -167,6 +180,7 @@ export function parsePrResponse(
167180
number: entry.number,
168181
state: entry.state.toLowerCase() as PrEntry["state"],
169182
url: entry.url,
183+
updatedAt: entry.updatedAt ?? "",
170184
ciStatus: summarizeChecks(entry.statusCheckRollup),
171185
ciChecks: mapChecks(entry.statusCheckRollup),
172186
comments: (entry.comments ?? []).map((c) => ({
@@ -200,7 +214,7 @@ export async function fetchAllPrs(
200214
"--state",
201215
"open",
202216
"--json",
203-
"number,headRefName,state,statusCheckRollup,url,comments",
217+
"number,headRefName,state,updatedAt,statusCheckRollup,url,comments",
204218
"--limit",
205219
String(PR_FETCH_LIMIT),
206220
];
@@ -258,7 +272,8 @@ export async function mapWithConcurrency<T, R>(
258272
return results;
259273
}
260274

261-
/** Fetch inline review comments for a single PR via `gh api`. Returns [] on error. */
275+
/** Fetch inline review comments for a single PR via `gh api` with ETag caching.
276+
* Conditional requests (304) don't count against GitHub's rate limit. */
262277
async function fetchReviewComments(
263278
prNumber: number,
264279
repoSlug?: string,
@@ -267,12 +282,18 @@ async function fetchReviewComments(
267282
const repoFlag = repoSlug
268283
? repoSlug
269284
: "{owner}/{repo}";
285+
const apiPath = `repos/${repoFlag}/pulls/${prNumber}/comments?per_page=100`;
270286
const args = [
271287
"gh", "api",
272-
`repos/${repoFlag}/pulls/${prNumber}/comments`,
273-
"--paginate",
288+
apiPath,
289+
"--include",
274290
];
275291

292+
const cached = etagCache.get(apiPath);
293+
if (cached) {
294+
args.push("--header", `If-None-Match: ${cached.etag}`);
295+
}
296+
276297
const proc = Bun.spawn(args, {
277298
stdout: "pipe",
278299
stderr: "pipe",
@@ -285,13 +306,49 @@ async function fetchReviewComments(
285306
});
286307

287308
const raceResult = await Promise.race([proc.exited, timeout]);
288-
if (raceResult === "timeout" || raceResult !== 0) return [];
309+
if (raceResult === "timeout") return cached?.comments ?? [];
310+
311+
const raw = await new Response(proc.stdout).text();
312+
313+
// gh api --include prefixes the body with HTTP headers separated by a blank line
314+
let blankLineIdx = raw.indexOf("\r\n\r\n");
315+
let separatorLen = 4;
316+
if (blankLineIdx === -1) {
317+
blankLineIdx = raw.indexOf("\n\n");
318+
separatorLen = 2;
319+
}
320+
if (blankLineIdx === -1) {
321+
// No headers found — may be an error or empty response
322+
if (raceResult !== 0) return cached?.comments ?? [];
323+
try {
324+
return parseReviewComments(raw);
325+
} catch {
326+
return cached?.comments ?? [];
327+
}
328+
}
329+
330+
const headerBlock = raw.slice(0, blankLineIdx);
331+
const body = raw.slice(blankLineIdx + separatorLen);
332+
333+
// Check for 304 Not Modified
334+
if (headerBlock.includes("304 Not Modified")) {
335+
log.debug(`[pr] etag cache hit for PR #${prNumber}`);
336+
return cached?.comments ?? [];
337+
}
338+
339+
if (raceResult !== 0) return cached?.comments ?? [];
340+
341+
// Parse ETag from response headers
342+
const etagMatch = headerBlock.match(/^etag:\s*(.+)$/mi);
289343

290344
try {
291-
const json = await new Response(proc.stdout).text();
292-
return parseReviewComments(json);
345+
const comments = parseReviewComments(body);
346+
if (etagMatch) {
347+
etagCache.set(apiPath, { etag: etagMatch[1].trim(), comments });
348+
}
349+
return comments;
293350
} catch {
294-
return [];
351+
return cached?.comments ?? [];
295352
}
296353
}
297354

@@ -350,6 +407,7 @@ export async function syncPrStatus(
350407
linkedRepos: LinkedRepoConfig[],
351408
projectDir?: string,
352409
): Promise<void> {
410+
log.debug(`[pr] starting sync (${1 + linkedRepos.length} repo(s))`);
353411
// Fetch current repo + all linked repos in parallel.
354412
const allRepoResults = await Promise.all([
355413
fetchAllPrs(undefined, undefined, projectDir),
@@ -370,12 +428,20 @@ export async function syncPrStatus(
370428
}
371429
}
372430

373-
// Fetch inline review comments for all open PRs (concurrency-limited)
374-
// and merge into comments array, sorted by date.
431+
// Fetch inline review comments for open PRs whose updatedAt has changed.
432+
// PRs that haven't been updated reuse cached comments (saves API calls).
375433
const reviewTuples: { entry: PrEntry; repoSlug: string | undefined }[] = [];
376434
for (const entries of branchPrs.values()) {
377435
for (const entry of entries) {
378-
if (entry.state === "open") {
436+
if (entry.state !== "open") continue;
437+
const cachedUpdatedAt = prUpdatedAtCache.get(entry.url);
438+
if (cachedUpdatedAt === entry.updatedAt && prCommentsCache.has(entry.url)) {
439+
log.debug(`[pr] skipping comments for PR #${entry.number} (unchanged)`);
440+
const cached = prCommentsCache.get(entry.url)!;
441+
entry.comments = [...entry.comments, ...cached].sort(
442+
(a, b) => new Date(a.createdAt).getTime() - new Date(b.createdAt).getTime(),
443+
);
444+
} else {
379445
const repoSlug = entry.repo
380446
? linkedRepos.find((lr) => lr.alias === entry.repo)?.repo
381447
: undefined;
@@ -384,12 +450,16 @@ export async function syncPrStatus(
384450
}
385451
}
386452
if (reviewTuples.length > 0) {
453+
log.debug(`[pr] fetching review comments for ${reviewTuples.length} PR(s)`);
387454
const reviewResults = await mapWithConcurrency(reviewTuples, 5, (t) =>
388455
fetchReviewComments(t.entry.number, t.repoSlug, projectDir),
389456
);
390457
for (let i = 0; i < reviewTuples.length; i++) {
391458
const entry = reviewTuples[i].entry;
392-
entry.comments = [...entry.comments, ...reviewResults[i]].sort(
459+
const reviewComments = reviewResults[i];
460+
prUpdatedAtCache.set(entry.url, entry.updatedAt);
461+
prCommentsCache.set(entry.url, reviewComments);
462+
entry.comments = [...entry.comments, ...reviewComments].sort(
393463
(a, b) => new Date(a.createdAt).getTime() - new Date(b.createdAt).getTime(),
394464
);
395465
}
@@ -421,16 +491,44 @@ export async function syncPrStatus(
421491
staleRefreshes.push(refreshStalePrData(wtDir));
422492
}
423493
await Promise.all(staleRefreshes);
494+
495+
// Evict cache entries for PRs that are no longer open.
496+
const currentPrUrls = new Set<string>();
497+
const currentApiPaths = new Set<string>();
498+
for (const entries of branchPrs.values()) {
499+
for (const entry of entries) {
500+
currentPrUrls.add(entry.url);
501+
const repoSlug = entry.repo
502+
? linkedRepos.find((lr) => lr.alias === entry.repo)?.repo ?? "{owner}/{repo}"
503+
: "{owner}/{repo}";
504+
currentApiPaths.add(`repos/${repoSlug}/pulls/${entry.number}/comments?per_page=100`);
505+
}
506+
}
507+
for (const url of prUpdatedAtCache.keys()) {
508+
if (!currentPrUrls.has(url)) prUpdatedAtCache.delete(url);
509+
}
510+
for (const url of prCommentsCache.keys()) {
511+
if (!currentPrUrls.has(url)) prCommentsCache.delete(url);
512+
}
513+
for (const key of etagCache.keys()) {
514+
if (!currentApiPaths.has(key)) etagCache.delete(key);
515+
}
424516
}
425517

426-
/** Start periodic PR status sync. Returns a cleanup function that stops the monitor. */
518+
/** Start periodic PR status sync. Returns a cleanup function that stops the monitor.
519+
* When `isActive` is provided, polling is skipped if no clients are connected. */
427520
export function startPrMonitor(
428521
getWorktreePaths: () => Promise<Map<string, string>>,
429522
linkedRepos: LinkedRepoConfig[],
430523
projectDir?: string,
431524
intervalMs: number = 20_000,
525+
isActive?: () => boolean,
432526
): () => void {
433527
const run = (): void => {
528+
if (isActive && !isActive()) {
529+
log.debug("[pr] skipping PR sync: no active clients");
530+
return;
531+
}
434532
syncPrStatus(getWorktreePaths, linkedRepos, projectDir).catch(
435533
(err: unknown) => {
436534
log.error(`[pr] sync error: ${err}`);

backend/src/server.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ import { loadConfig, gitRoot, type WmdevConfig } from "./config";
3131
import { startPrMonitor, type PrEntry } from "./pr";
3232
import { handleWorkmuxRpc } from "./rpc";
3333
import { jsonResponse, errorResponse } from "./http";
34-
import { handleNotificationStream, handleDismissNotification, installHookScripts } from "./notifications";
34+
import { handleNotificationStream, handleDismissNotification, installHookScripts, hasDashboardActivity, touchActivity } from "./notifications";
3535
import { fetchAssignedIssues, branchMatchesIssue, type LinkedLinearIssue } from "./linear";
3636

3737
const PORT = parseInt(Bun.env.BACKEND_PORT || "5111", 10);
@@ -211,6 +211,7 @@ function makeCallbacks(ws: { send: (data: string) => void; readyState: number })
211211
// --- API handler functions (thin I/O layer, testable by injecting deps) ---
212212

213213
async function apiGetWorktrees(req: Request): Promise<Response> {
214+
touchActivity();
214215
const now = Date.now();
215216

216217
// Serve from cache if still fresh
@@ -610,7 +611,7 @@ if (tmuxCheck.exitCode !== 0) {
610611
}
611612

612613
cleanupStaleSessions();
613-
startPrMonitor(getWorktreePaths, config.linkedRepos, PROJECT_DIR);
614+
startPrMonitor(getWorktreePaths, config.linkedRepos, PROJECT_DIR, undefined, hasDashboardActivity);
614615
installHookScripts().catch((err: unknown) => {
615616
log.error(`[notify] failed to install hook scripts: ${err instanceof Error ? err.message : String(err)}`);
616617
});

frontend/src/App.svelte

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -280,25 +280,37 @@
280280
.catch(() => {});
281281
refresh();
282282
refreshLinear();
283-
const interval = setInterval(refresh, 5000);
283+
let interval = setInterval(refresh, 5000);
284284
window.addEventListener("keydown", handleKeydown);
285-
const unsubNotifications = api.subscribeNotifications(handleNotification, handleSseDismiss, handleInitialNotification);
285+
let unsubNotifications = api.subscribeNotifications(handleNotification, handleSseDismiss, handleInitialNotification);
286286
// Request notification permission (no-op if already granted/denied)
287287
if (Notification.permission === "default") {
288288
Notification.requestPermission().catch(() => {});
289289
}
290290
291+
// Pause polling when tab is hidden to reduce server load.
292+
function onVisibilityChange(): void {
293+
if (document.hidden) {
294+
clearInterval(interval);
295+
} else {
296+
refresh();
297+
interval = setInterval(refresh, 5000);
298+
}
299+
}
300+
document.addEventListener("visibilitychange", onVisibilityChange);
301+
291302
const mq = window.matchMedia("(max-width: 768px)");
292303
isMobile = mq.matches;
293304
if (isMobile) sidebarOpen = true;
294-
function onMqChange(e: MediaQueryListEvent) {
305+
function onMqChange(e: MediaQueryListEvent): void {
295306
isMobile = e.matches;
296307
}
297308
mq.addEventListener("change", onMqChange);
298309
299310
return () => {
300311
clearInterval(interval);
301312
window.removeEventListener("keydown", handleKeydown);
313+
document.removeEventListener("visibilitychange", onVisibilityChange);
302314
mq.removeEventListener("change", onMqChange);
303315
unsubNotifications();
304316
};

0 commit comments

Comments
 (0)