Skip to content

Commit 91dcf41

Browse files
authored
fix(rate-limit): fixes footer display and tracks GraphQL cost (#80)
* fix(github): route rate limit signals by pool on error paths - Gate updateRateLimitFromHeaders on !isGraphql to prevent GraphQL response headers from contaminating _coreRateLimit signal - Add GraphQL-aware signal updates to hook.wrap error path: routes GraphQL errors to _setGraphqlRateLimit, REST errors to updateRateLimitFromHeaders, wrapped in try/catch to never mask errors - Make fetchRateLimitDetails() update both signals on all return paths (cache-hit and fresh-fetch) - Add tests for signal routing, error path updates, and fetchRateLimitDetails signal feedback * fix(poll): seed rate limit signals at poll cycle start - Call fetchRateLimitDetails() after setIsRefreshing(true) to seed both rate limit signals with authoritative GET /rate_limit data - Add github module mock to poll tests for fetchRateLimitDetails - Add flushPromises helper for multi-await microtask flushing * fix(api): extracts rateLimit from error-path responses - runForkPRFallback: extract rateLimit from err.data before partialData - fetchPREnrichment: extract rateLimit at start of catch block - fetchHotPRStatus: extract from s.reason.data in rejection loop - Add tests for all three error-path extractions * fix(dashboard): adds error state when rate limit exhausted - Three-tier CSS class: text-error at remaining===0, text-warning at <10%, normal otherwise - Add DashboardPage.test.tsx with tests for all three styling states * fix(github): guards remaining against NaN, trims cache path - NaN-guard parseInt(remaining) in error-path signal update - Remove redundant signal updates from fetchRateLimitDetails cache hit path (signals already set on fresh fetch) - Add explanatory comment to flushPromises test helper * feat(api-usage): tracks GraphQL query point cost, not call count - Adds cost field to all 7 GraphQL rateLimit query fragments - Extracts rateLimit.cost from GraphQL response body in hook.wrap - Extracts cost from error response body on GraphQL failures - Passes graphqlCost via ApiRequestInfo to api-usage tracking - Renames Settings UI labels from Calls/counts to Usage - Add tests for graphqlCost extraction in hook.wrap callbacks * fix(rate-limit): address pr-review findings - Restore cache-hit signal updates in fetchRateLimitDetails - Extract rateLimitCssClass utility from inline ternary - Run fetchRateLimitDetails concurrently (void, not await) - Add error-path graphqlCost and call-count tests - Add fetchRateLimitDetails signal update + cache-hit tests - Fix section comment numbering cascade in SettingsPage - Add clientOverride param to fetchRateLimitDetails for testability
1 parent 3d73780 commit 91dcf41

12 files changed

Lines changed: 529 additions & 46 deletions

File tree

src/app/components/dashboard/DashboardPage.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import { expireToken, user, onAuthCleared, DASHBOARD_STORAGE_KEY } from "../../s
2626
import { updateRelaySnapshot } from "../../lib/mcp-relay";
2727
import { pushNotification } from "../../lib/errors";
2828
import { getClient, getGraphqlRateLimit, fetchRateLimitDetails } from "../../services/github";
29-
import { formatCount, prSizeCategory } from "../../lib/format";
29+
import { formatCount, prSizeCategory, rateLimitCssClass } from "../../lib/format";
3030
import { setsEqual } from "../../lib/collections";
3131
import { withScrollLock } from "../../lib/scroll";
3232
import { Tooltip } from "../shared/Tooltip";
@@ -926,7 +926,7 @@ export default function DashboardPage() {
926926
{(rl) => (
927927
<div onPointerEnter={fetchAndSetRlDetail} onFocusIn={fetchAndSetRlDetail}>
928928
<Tooltip content={rlDetail()} placement="left" focusable contentClass="whitespace-pre font-mono text-xs">
929-
<span class={`tabular-nums ${rl().remaining < rl().limit * 0.1 ? "text-warning" : ""}`}>
929+
<span class={`tabular-nums ${rateLimitCssClass(rl().remaining, rl().limit)}`}>
930930
API RL: {rl().remaining.toLocaleString()}/{formatCount(rl().limit)}/hr
931931
</span>
932932
</Tooltip>

src/app/components/settings/SettingsPage.tsx

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,7 @@ export default function SettingsPage() {
422422
<tr>
423423
<th>Source</th>
424424
<th>Pool</th>
425-
<th>Calls</th>
425+
<th>Usage</th>
426426
<th>Last Called</th>
427427
</tr>
428428
</thead>
@@ -468,7 +468,7 @@ export default function SettingsPage() {
468468
onClick={() => resetUsageData()}
469469
class="btn btn-xs btn-ghost"
470470
>
471-
Reset counts
471+
Reset usage
472472
</button>
473473
</div>
474474
</div>
@@ -514,7 +514,7 @@ export default function SettingsPage() {
514514
</SettingRow>
515515
</Section>
516516

517-
{/* Section 5: Notifications */}
517+
{/* Section 6: Notifications */}
518518
<Section title="Notifications">
519519
<SettingRow
520520
label="Enable notifications"
@@ -606,7 +606,7 @@ export default function SettingsPage() {
606606
</SettingRow>
607607
</Section>
608608

609-
{/* Section 6: Appearance */}
609+
{/* Section 7: Appearance */}
610610
<Section title="Appearance">
611611
<div class="px-4 py-2 border-b border-base-300">
612612
<p class="text-sm font-medium text-base-content mb-2">Theme</p>
@@ -634,7 +634,7 @@ export default function SettingsPage() {
634634
</SettingRow>
635635
</Section>
636636

637-
{/* Section 7: Tabs */}
637+
{/* Section 8: Tabs */}
638638
<Section title="Tabs">
639639
<SettingRow
640640
label="Default tab"
@@ -691,15 +691,15 @@ export default function SettingsPage() {
691691
</SettingRow>
692692
</Section>
693693

694-
{/* Section 8: Custom Tabs */}
694+
{/* Section 9: Custom Tabs */}
695695
<Section title="Custom Tabs" description="Create custom views with saved filters and scoping">
696696
<CustomTabsSection
697697
availableOrgs={[...new Set(config.selectedRepos.map((r) => r.owner))]}
698698
availableRepos={config.selectedRepos}
699699
/>
700700
</Section>
701701

702-
{/* Section 9: MCP Server Relay */}
702+
{/* Section 10: MCP Server Relay */}
703703
<Section
704704
title="MCP Server Relay"
705705
description="Allow a local MCP server to read dashboard data. Enable this if you use Claude Code or another AI client with the GitHub Tracker MCP server."
@@ -754,7 +754,7 @@ export default function SettingsPage() {
754754
</Show>
755755
</Section>
756756

757-
{/* Section 10: Data */}
757+
{/* Section 11: Data */}
758758
<Section title="Data">
759759
{/* Authentication method */}
760760
<SettingRow

src/app/lib/format.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
// Re-exports from shared/format for backward compat with existing importers.
22
export { relativeTime, shortRelativeTime, labelTextColor, formatDuration, prSizeCategory, deriveInvolvementRoles, formatCount, formatStarCount } from "../../shared/format";
33

4+
export function rateLimitCssClass(remaining: number, limit: number): string {
5+
if (remaining === 0) return "text-error";
6+
if (remaining < limit * 0.1) return "text-warning";
7+
return "";
8+
}
9+
410
/** Format scope counts as "N org(s), M repo(s)". When elideZero is true, omit zero-count segments. */
511
export function formatScopeSummary(orgCount: number, repoCount: number, elideZero = false): string {
612
if (orgCount === 0 && repoCount === 0) return "All repos";

src/app/services/api-usage.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ export function deriveSource(info: ApiRequestInfo): ApiCallSource {
224224
onApiRequest((info) => {
225225
const source = deriveSource(info);
226226
const pool: ApiPool = info.isGraphql ? "graphql" : "core";
227-
trackApiCall(source, pool);
227+
trackApiCall(source, pool, info.graphqlCost ?? 1);
228228
// Both pools tracked — Math.max keeps latest; may delay reset of the earlier pool's records
229229
if (info.resetEpochMs !== null) updateResetAt(info.resetEpochMs);
230230
});

src/app/services/api.ts

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ export type { Issue, PullRequest, WorkflowRun, RepoRef, RepoEntry, OrgEntry, Che
1212
// ── Types ────────────────────────────────────────────────────────────────────
1313

1414
interface GraphQLRateLimit {
15+
cost?: number;
1516
limit: number;
1617
remaining: number;
1718
resetAt: string;
@@ -226,7 +227,7 @@ const ISSUES_SEARCH_QUERY = `
226227
}
227228
}
228229
}
229-
rateLimit { limit remaining resetAt }
230+
rateLimit { cost limit remaining resetAt }
230231
}
231232
${LIGHT_ISSUE_FRAGMENT}
232233
`;
@@ -287,7 +288,7 @@ const LIGHT_COMBINED_SEARCH_QUERY = `
287288
}
288289
}
289290
}
290-
rateLimit { limit remaining resetAt }
291+
rateLimit { cost limit remaining resetAt }
291292
}
292293
${LIGHT_ISSUE_FRAGMENT}
293294
${LIGHT_PR_FRAGMENT}
@@ -318,7 +319,7 @@ const UNFILTERED_SEARCH_QUERY = `
318319
}
319320
}
320321
}
321-
rateLimit { limit remaining resetAt }
322+
rateLimit { cost limit remaining resetAt }
322323
}
323324
${LIGHT_ISSUE_FRAGMENT}
324325
${LIGHT_PR_FRAGMENT}
@@ -350,7 +351,7 @@ const LIGHT_PR_SEARCH_QUERY = `
350351
}
351352
}
352353
}
353-
rateLimit { limit remaining resetAt }
354+
rateLimit { cost limit remaining resetAt }
354355
}
355356
${LIGHT_PR_FRAGMENT}
356357
`;
@@ -395,7 +396,7 @@ const HEAVY_PR_BACKFILL_QUERY = `
395396
}
396397
}
397398
}
398-
rateLimit { limit remaining resetAt }
399+
rateLimit { cost limit remaining resetAt }
399400
}
400401
`;
401402

@@ -417,7 +418,7 @@ const HOT_PR_STATUS_QUERY = `
417418
}
418419
}
419420
}
420-
rateLimit { limit remaining resetAt }
421+
rateLimit { cost limit remaining resetAt }
421422
}
422423
`;
423424

@@ -673,7 +674,7 @@ async function runForkPRFallback(
673674
);
674675
}
675676

676-
const forkQuery = `query(${varDefs.join(", ")}) {\n${fragments.join("\n")}\nrateLimit { limit remaining resetAt }\n}`;
677+
const forkQuery = `query(${varDefs.join(", ")}) {\n${fragments.join("\n")}\nrateLimit { cost limit remaining resetAt }\n}`;
677678

678679
try {
679680
const forkResponse = await octokit.graphql<ForkQueryResponse>(forkQuery, { ...variables, request: { apiSource: "forkCheck" } });
@@ -688,9 +689,13 @@ async function runForkPRFallback(
688689
if (pr) pr.checkStatus = mapCheckStatus(state);
689690
}
690691
} catch (err) {
691-
const partialData = (err && typeof err === "object" && "data" in err && err.data && typeof err.data === "object")
692-
? err.data as Record<string, ForkRepoResult | null | undefined>
692+
const errDataRaw = (err && typeof err === "object" && "data" in err && err.data && typeof err.data === "object")
693+
? err.data as Record<string, unknown>
693694
: null;
695+
if (errDataRaw?.rateLimit) {
696+
updateGraphqlRateLimit(errDataRaw.rateLimit as GraphQLRateLimit);
697+
}
698+
const partialData = errDataRaw as Record<string, ForkRepoResult | null | undefined> | null;
694699

695700
if (partialData) {
696701
for (let i = 0; i < forkChunk.length; i++) {
@@ -1130,6 +1135,12 @@ export async function fetchPREnrichment(
11301135
});
11311136
}
11321137
} catch (err) {
1138+
const partialErr = (err && typeof err === "object" && "data" in err && err.data && typeof err.data === "object")
1139+
? err.data as Partial<HeavyBackfillResponse>
1140+
: null;
1141+
if (partialErr?.rateLimit) {
1142+
updateGraphqlRateLimit(partialErr.rateLimit);
1143+
}
11331144
const { statusCode, message } = extractRejectionError(err);
11341145
errors.push({
11351146
repo: `backfill-batch-${batchIdx + 1}/${batches.length}`,
@@ -1686,6 +1697,13 @@ export async function fetchHotPRStatus(
16861697
hadErrors = true;
16871698
console.warn("[hot-poll] PR status batch failed:", s.reason);
16881699
Sentry.captureException(s.reason, { tags: { source: "hot-poll-pr-batch" } });
1700+
const reason = s.reason;
1701+
const partialErr = (reason && typeof reason === "object" && "data" in reason && reason.data && typeof reason.data === "object")
1702+
? reason.data as Partial<HotPRStatusResponse>
1703+
: null;
1704+
if (partialErr?.rateLimit) {
1705+
updateGraphqlRateLimit(partialErr.rateLimit);
1706+
}
16891707
}
16901708
}
16911709

src/app/services/github.ts

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ export interface ApiRequestInfo {
6767
apiSource?: string;
6868
/** x-ratelimit-reset converted to ms, or null if unavailable */
6969
resetEpochMs: number | null;
70+
/** GraphQL query point cost from response body, or undefined for REST */
71+
graphqlCost?: number;
7072
}
7173

7274
const _requestCallbacks: Array<(info: ApiRequestInfo) => void> = [];
@@ -147,29 +149,57 @@ export function createGitHubClient(token: string): GitHubOctokitInstance {
147149
// Fire callbacks even on errors — these are real API calls.
148150
// Octokit's RequestError includes response.headers for HTTP errors (403, 404, etc.)
149151
// so we can still extract x-ratelimit-reset when available.
152+
const errResponse = (err as { response?: { headers?: Record<string, string> } }).response;
150153
if (status > 0) {
151154
let resetEpochMs: number | null = null;
152-
const errResponse = (err as { response?: { headers?: Record<string, string> } }).response;
153155
const errResetHeader = errResponse?.headers?.["x-ratelimit-reset"];
154156
if (errResetHeader) resetEpochMs = parseInt(errResetHeader, 10) * 1000;
157+
// Octokit errors store data in two shapes: err.data has the unwrapped GraphQL
158+
// data object, while errResponse.data is the raw HTTP body (GraphQL envelope)
159+
const errGraphqlCost = isGraphql
160+
? ((err as { data?: { rateLimit?: { cost?: number } } }).data?.rateLimit?.cost
161+
?? (errResponse as { data?: { data?: { rateLimit?: { cost?: number } } } } | undefined)?.data?.data?.rateLimit?.cost)
162+
: undefined;
155163
const info: ApiRequestInfo = {
156-
url: options.url, method, status, isGraphql, apiSource, resetEpochMs,
164+
url: options.url, method, status, isGraphql, apiSource, resetEpochMs, graphqlCost: errGraphqlCost,
157165
};
158166
for (const cb of _requestCallbacks) { try { cb(info); } catch { /* swallow */ } }
159167
}
168+
const errHeaders = errResponse?.headers as Record<string, string> | undefined;
169+
if (errHeaders) {
170+
try {
171+
if (isGraphql) {
172+
const remaining = errHeaders["x-ratelimit-remaining"];
173+
const reset = errHeaders["x-ratelimit-reset"];
174+
const limit = errHeaders["x-ratelimit-limit"];
175+
if (remaining !== undefined && reset !== undefined) {
176+
_setGraphqlRateLimit({
177+
limit: safePositiveNumber(limit !== undefined ? parseInt(limit, 10) : NaN, _graphqlRateLimit()?.limit ?? 5000),
178+
remaining: Number.isFinite(parseInt(remaining, 10)) ? parseInt(remaining, 10) : 0,
179+
resetAt: new Date(parseInt(reset, 10) * 1000),
180+
});
181+
}
182+
} else {
183+
updateRateLimitFromHeaders(errHeaders);
184+
}
185+
} catch { /* never mask the original error */ }
186+
}
160187
throw err;
161188
}
162189

163190
// Success path — fire callbacks (api-usage.ts registers at module scope) and update RL display
164191
const headers = (response.headers ?? {}) as Record<string, string>;
165192
const resetHeader = headers["x-ratelimit-reset"];
166193
const resetEpochMs = resetHeader ? parseInt(resetHeader, 10) * 1000 : null;
194+
const graphqlCost = isGraphql
195+
? (response.data as { data?: { rateLimit?: { cost?: number } } })?.data?.rateLimit?.cost ?? undefined
196+
: undefined;
167197
const info: ApiRequestInfo = {
168-
url: options.url, method, status, isGraphql, apiSource, resetEpochMs,
198+
url: options.url, method, status, isGraphql, apiSource, resetEpochMs, graphqlCost,
169199
};
170200
for (const cb of _requestCallbacks) { try { cb(info); } catch { /* swallow */ } }
171201

172-
if (response.headers) {
202+
if (response.headers && !isGraphql) {
173203
updateRateLimitFromHeaders(response.headers as Record<string, string>);
174204
}
175205

@@ -283,13 +313,15 @@ let _lastFetchResult: { core: RateLimitInfo; graphql: RateLimitInfo } | null = n
283313
* GET /rate_limit is free — not counted against rate limits by GitHub.
284314
* Returns null if client unavailable or request fails.
285315
*/
286-
export async function fetchRateLimitDetails(): Promise<{ core: RateLimitInfo; graphql: RateLimitInfo } | null> {
316+
export async function fetchRateLimitDetails(clientOverride?: GitHubOctokitInstance): Promise<{ core: RateLimitInfo; graphql: RateLimitInfo } | null> {
287317
// Return cached result within 5-second staleness window
288318
if (_lastFetchResult !== null && Date.now() - _lastFetchTime < 5000) {
319+
_setCoreRateLimit(_lastFetchResult.core);
320+
_setGraphqlRateLimit(_lastFetchResult.graphql);
289321
return { ..._lastFetchResult };
290322
}
291323

292-
const client = getClient();
324+
const client = clientOverride ?? getClient();
293325
if (!client) return null;
294326

295327
try {
@@ -310,6 +342,8 @@ export async function fetchRateLimitDetails(): Promise<{ core: RateLimitInfo; gr
310342
};
311343
_lastFetchTime = Date.now();
312344
_lastFetchResult = result;
345+
_setCoreRateLimit(result.core);
346+
_setGraphqlRateLimit(result.graphql);
313347
return { ...result };
314348
} catch {
315349
return null;

src/app/services/poll.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { createSignal, createEffect, createRoot, untrack, onCleanup } from "solid-js";
22
import * as Sentry from "@sentry/solid";
3-
import { getClient } from "./github";
3+
import { getClient, fetchRateLimitDetails } from "./github";
44
import { config } from "../stores/config";
55
import { user, onAuthCleared } from "../stores/auth";
66
import { checkAndResetIfExpired } from "./api-usage";
@@ -357,6 +357,9 @@ export function createPollCoordinator(
357357
if (destroyed || isRefreshing()) return;
358358
checkAndResetIfExpired();
359359
setIsRefreshing(true);
360+
// Fire-and-forget: seeds footer signals concurrently with fetchAll. If GET /rate_limit
361+
// resolves after a GraphQL response, the footer briefly shows pre-query remaining (cosmetic).
362+
void fetchRateLimitDetails();
360363

361364
// Snapshot sources of notifications from previous cycle (for reconciliation)
362365
const previousSources = new Set(
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import { describe, it, expect } from "vitest";
2+
import { rateLimitCssClass } from "../../../src/app/lib/format";
3+
4+
describe("rateLimitCssClass", () => {
5+
it("remaining: 0 gives text-error", () => {
6+
expect(rateLimitCssClass(0, 5000)).toBe("text-error");
7+
});
8+
9+
it("remaining < 10% of limit gives text-warning", () => {
10+
expect(rateLimitCssClass(100, 5000)).toBe("text-warning");
11+
});
12+
13+
it("remaining >= 10% of limit gives empty string", () => {
14+
expect(rateLimitCssClass(3000, 5000)).toBe("");
15+
});
16+
17+
it("remaining exactly at 10% threshold gives empty string (strict less-than)", () => {
18+
expect(rateLimitCssClass(500, 5000)).toBe("");
19+
});
20+
21+
it("remaining just below 10% threshold gives text-warning", () => {
22+
expect(rateLimitCssClass(499, 5000)).toBe("text-warning");
23+
});
24+
});

tests/components/settings/ApiUsageSection.test.tsx

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -274,18 +274,18 @@ describe("ApiUsageSection — reset button", () => {
274274
mockGetUsageResetAt.mockReturnValue(null);
275275
});
276276

277-
it("renders the 'Reset counts' button", () => {
277+
it("renders the 'Reset usage' button", () => {
278278
renderSettings();
279-
expect(screen.getByText("Reset counts")).toBeTruthy();
279+
expect(screen.getByText("Reset usage")).toBeTruthy();
280280
});
281281

282-
it("calls resetUsageData() when 'Reset counts' button is clicked", () => {
282+
it("calls resetUsageData() when 'Reset usage' button is clicked", () => {
283283
// Wire the mock to clear snapshot on reset, simulating real behavior
284284
mockResetUsageData.mockImplementation(() => {
285285
mockGetUsageSnapshot.mockReturnValue([]);
286286
});
287287
renderSettings();
288-
const btn = screen.getByText("Reset counts");
288+
const btn = screen.getByText("Reset usage");
289289
fireEvent.click(btn);
290290
expect(mockResetUsageData).toHaveBeenCalledOnce();
291291
});

0 commit comments

Comments
 (0)