From 3c80a0876c6f8d1c794bfe209a05fe318689d0c8 Mon Sep 17 00:00:00 2001 From: testvalue Date: Thu, 23 Apr 2026 08:40:22 -0400 Subject: [PATCH 1/7] 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 --- src/app/services/github.ts | 27 +++++- tests/services/github.test.ts | 168 +++++++++++++++++++++++++++++++++- 2 files changed, 192 insertions(+), 3 deletions(-) diff --git a/src/app/services/github.ts b/src/app/services/github.ts index 6b27dc67..c6d0c55f 100644 --- a/src/app/services/github.ts +++ b/src/app/services/github.ts @@ -147,9 +147,9 @@ export function createGitHubClient(token: string): GitHubOctokitInstance { // Fire callbacks even on errors — these are real API calls. // Octokit's RequestError includes response.headers for HTTP errors (403, 404, etc.) // so we can still extract x-ratelimit-reset when available. + const errResponse = (err as { response?: { headers?: Record } }).response; if (status > 0) { let resetEpochMs: number | null = null; - const errResponse = (err as { response?: { headers?: Record } }).response; const errResetHeader = errResponse?.headers?.["x-ratelimit-reset"]; if (errResetHeader) resetEpochMs = parseInt(errResetHeader, 10) * 1000; const info: ApiRequestInfo = { @@ -157,6 +157,25 @@ export function createGitHubClient(token: string): GitHubOctokitInstance { }; for (const cb of _requestCallbacks) { try { cb(info); } catch { /* swallow */ } } } + const errHeaders = errResponse?.headers as Record | undefined; + if (errHeaders) { + try { + if (isGraphql) { + const remaining = errHeaders["x-ratelimit-remaining"]; + const reset = errHeaders["x-ratelimit-reset"]; + const limit = errHeaders["x-ratelimit-limit"]; + if (remaining !== undefined && reset !== undefined) { + _setGraphqlRateLimit({ + limit: safePositiveNumber(limit !== undefined ? parseInt(limit, 10) : NaN, _graphqlRateLimit()?.limit ?? 5000), + remaining: parseInt(remaining, 10), + resetAt: new Date(parseInt(reset, 10) * 1000), + }); + } + } else { + updateRateLimitFromHeaders(errHeaders); + } + } catch { /* never mask the original error */ } + } throw err; } @@ -169,7 +188,7 @@ export function createGitHubClient(token: string): GitHubOctokitInstance { }; for (const cb of _requestCallbacks) { try { cb(info); } catch { /* swallow */ } } - if (response.headers) { + if (response.headers && !isGraphql) { updateRateLimitFromHeaders(response.headers as Record); } @@ -286,6 +305,8 @@ let _lastFetchResult: { core: RateLimitInfo; graphql: RateLimitInfo } | null = n export async function fetchRateLimitDetails(): Promise<{ core: RateLimitInfo; graphql: RateLimitInfo } | null> { // Return cached result within 5-second staleness window if (_lastFetchResult !== null && Date.now() - _lastFetchTime < 5000) { + _setCoreRateLimit(_lastFetchResult.core); + _setGraphqlRateLimit(_lastFetchResult.graphql); return { ..._lastFetchResult }; } @@ -310,6 +331,8 @@ export async function fetchRateLimitDetails(): Promise<{ core: RateLimitInfo; gr }; _lastFetchTime = Date.now(); _lastFetchResult = result; + _setCoreRateLimit(result.core); + _setGraphqlRateLimit(result.graphql); return { ...result }; } catch { return null; diff --git a/tests/services/github.test.ts b/tests/services/github.test.ts index 4f462329..de9c3359 100644 --- a/tests/services/github.test.ts +++ b/tests/services/github.test.ts @@ -1,7 +1,7 @@ import "fake-indexeddb/auto"; import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { createRoot } from "solid-js"; -import { createGitHubClient, cachedRequest, getClient, initClientWatcher, getGraphqlRateLimit, updateGraphqlRateLimit, updateRateLimitFromHeaders, getCoreRateLimit, onApiRequest, type ApiRequestInfo } from "../../src/app/services/github"; +import { createGitHubClient, cachedRequest, getClient, initClientWatcher, getGraphqlRateLimit, updateGraphqlRateLimit, updateRateLimitFromHeaders, getCoreRateLimit, fetchRateLimitDetails, onApiRequest, type ApiRequestInfo } from "../../src/app/services/github"; import { clearCache } from "../../src/app/stores/cache"; // ── createGitHubClient ─────────────────────────────────────────────────────── @@ -569,3 +569,169 @@ describe("hook.wrap — request tracking callbacks", () => { expect(cbSpy).toHaveBeenCalled(); }); }); + +// ── hook.wrap — rate limit signal routing ──────────────────────────────────── + +describe("hook.wrap — rate limit signal routing", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + // Use status 500 (not 403/429) to avoid the throttle plugin's onRateLimit callback, + // which would schedule a real retry delay. hook.wrap extracts x-ratelimit-* headers + // for any error status, so 500 exercises the same signal-update code path. + it("GraphQL error response with rate limit headers updates _graphqlRateLimit", async () => { + const resetEpoch = Math.floor(Date.now() / 1000) + 3600; + vi.stubGlobal("fetch", vi.fn().mockResolvedValue( + new Response(JSON.stringify({ message: "internal server error" }), { + status: 500, + headers: { + "content-type": "application/json", + "x-ratelimit-remaining": "100", + "x-ratelimit-reset": String(resetEpoch), + "x-ratelimit-limit": "5000", + }, + }) + )); + + const client = createGitHubClient("test-token"); + await expect(client.graphql("{ viewer { login } }", { request: { retries: 0 } })).rejects.toThrow(); + + const rl = getGraphqlRateLimit(); + expect(rl).not.toBeNull(); + expect(rl!.remaining).toBe(100); + expect(rl!.limit).toBe(5000); + expect(rl!.resetAt).toBeInstanceOf(Date); + expect(rl!.resetAt.getTime()).toBe(resetEpoch * 1000); + }); + + it("REST error response with rate limit headers updates _coreRateLimit", async () => { + const resetEpoch = Math.floor(Date.now() / 1000) + 3600; + vi.stubGlobal("fetch", vi.fn().mockResolvedValue( + new Response(JSON.stringify({ message: "internal server error" }), { + status: 500, + headers: { + "content-type": "application/json", + "x-ratelimit-remaining": "100", + "x-ratelimit-reset": String(resetEpoch), + "x-ratelimit-limit": "5000", + }, + }) + )); + + const client = createGitHubClient("test-token"); + await expect(client.request("GET /user", { request: { retries: 0 } })).rejects.toThrow(); + + const rl = getCoreRateLimit(); + expect(rl).not.toBeNull(); + expect(rl!.remaining).toBe(100); + expect(rl!.limit).toBe(5000); + }); + + it("error response without rate limit headers does not crash", async () => { + vi.stubGlobal("fetch", vi.fn().mockResolvedValue( + new Response(JSON.stringify({ message: "not found" }), { + status: 404, + headers: { "content-type": "application/json" }, + }) + )); + + const client = createGitHubClient("test-token"); + // Must reject with the original error only, not an additional crash + await expect(client.request("GET /user")).rejects.toBeDefined(); + }); + + it("successful GraphQL request does NOT update _coreRateLimit", async () => { + const resetEpoch = Math.floor(Date.now() / 1000) + 3600; + // Seed _coreRateLimit to a known value + updateRateLimitFromHeaders({ + "x-ratelimit-remaining": "1234", + "x-ratelimit-reset": String(resetEpoch), + "x-ratelimit-limit": "5000", + }); + + vi.stubGlobal("fetch", vi.fn().mockResolvedValue( + new Response(JSON.stringify({ data: { viewer: { login: "test" } } }), { + status: 200, + headers: { + "content-type": "application/json", + "x-ratelimit-remaining": "9999", + "x-ratelimit-reset": String(resetEpoch), + "x-ratelimit-limit": "5000", + }, + }) + )); + + const client = createGitHubClient("test-token"); + await client.graphql("{ viewer { login } }"); + + // _coreRateLimit must remain at seeded value — GraphQL success must not contaminate it + const rl = getCoreRateLimit(); + expect(rl!.remaining).toBe(1234); + }); + + it("successful REST request updates _coreRateLimit but NOT _graphqlRateLimit", async () => { + const resetEpoch = Math.floor(Date.now() / 1000) + 3600; + // Seed _graphqlRateLimit to a known value + updateGraphqlRateLimit({ limit: 5000, remaining: 4321, resetAt: new Date(resetEpoch * 1000).toISOString() }); + + vi.stubGlobal("fetch", vi.fn().mockResolvedValue( + new Response(JSON.stringify({ login: "test" }), { + status: 200, + headers: { + "content-type": "application/json", + "x-ratelimit-remaining": "4888", + "x-ratelimit-reset": String(resetEpoch), + "x-ratelimit-limit": "5000", + }, + }) + )); + + const client = createGitHubClient("test-token"); + await client.request("GET /user"); + + const coreRl = getCoreRateLimit(); + expect(coreRl!.remaining).toBe(4888); + + // _graphqlRateLimit must be unchanged at seeded value + const graphqlRl = getGraphqlRateLimit(); + expect(graphqlRl!.remaining).toBe(4321); + }); +}); + +// ── fetchRateLimitDetails ──────────────────────────────────────────────────── +// fetchRateLimitDetails calls getClient() which reads an internal SolidJS signal +// (_client) that is only non-null after auth token is set. These tests exercise +// the null-client fast-path and the signal-preservation guarantee on failure. +// Signal update tests (setCoreRateLimit / setGraphqlRateLimit) are covered by the +// hook.wrap tests above which exercise the same signal setters via real HTTP responses. + +describe("fetchRateLimitDetails", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("returns null immediately when no client is available", async () => { + // No auth token is set in tests, so getClient() returns null → fetchRateLimitDetails returns null. + const result = await fetchRateLimitDetails(); + expect(result).toBeNull(); + }); + + it("failure does not clear existing signals", async () => { + // Seed signals to known values using the exported update functions. + const resetEpoch = Math.floor(Date.now() / 1000) + 3600; + updateRateLimitFromHeaders({ + "x-ratelimit-remaining": "1111", + "x-ratelimit-reset": String(resetEpoch), + "x-ratelimit-limit": "5000", + }); + updateGraphqlRateLimit({ limit: 5000, remaining: 2222, resetAt: new Date(resetEpoch * 1000).toISOString() }); + + // fetchRateLimitDetails returns null (no client) — signals must be unaffected. + const result = await fetchRateLimitDetails(); + expect(result).toBeNull(); + + expect(getCoreRateLimit()!.remaining).toBe(1111); + expect(getGraphqlRateLimit()!.remaining).toBe(2222); + }); +}); From 18c6aa3eed494f6d4a04f6c47991538dcd8d64db Mon Sep 17 00:00:00 2001 From: testvalue Date: Thu, 23 Apr 2026 08:41:09 -0400 Subject: [PATCH 2/7] 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 --- src/app/services/poll.ts | 3 ++- tests/services/poll.test.ts | 42 ++++++++++++++++++++++++------------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/app/services/poll.ts b/src/app/services/poll.ts index 5d44f32d..850777e6 100644 --- a/src/app/services/poll.ts +++ b/src/app/services/poll.ts @@ -1,6 +1,6 @@ import { createSignal, createEffect, createRoot, untrack, onCleanup } from "solid-js"; import * as Sentry from "@sentry/solid"; -import { getClient } from "./github"; +import { getClient, fetchRateLimitDetails } from "./github"; import { config } from "../stores/config"; import { user, onAuthCleared } from "../stores/auth"; import { checkAndResetIfExpired } from "./api-usage"; @@ -357,6 +357,7 @@ export function createPollCoordinator( if (destroyed || isRefreshing()) return; checkAndResetIfExpired(); setIsRefreshing(true); + await fetchRateLimitDetails(); // Snapshot sources of notifications from previous cycle (for reconciliation) const previousSources = new Set( diff --git a/tests/services/poll.test.ts b/tests/services/poll.test.ts index 87080234..cd4155b1 100644 --- a/tests/services/poll.test.ts +++ b/tests/services/poll.test.ts @@ -32,6 +32,14 @@ vi.mock("../../src/app/lib/notifications", () => ({ _resetNotificationState: vi.fn(), })); +// Mock github module — fetchRateLimitDetails adds an async boundary in doFetch +vi.mock("../../src/app/services/github", () => ({ + getClient: vi.fn(() => null), + fetchRateLimitDetails: vi.fn(() => Promise.resolve(null)), + onApiRequest: vi.fn(), + initClientWatcher: vi.fn(), +})); + // Mock config so doFetch doesn't fail when accessing config.selectedRepos vi.mock("../../src/app/stores/config", () => ({ config: { @@ -41,6 +49,10 @@ vi.mock("../../src/app/stores/config", () => ({ }, })); +async function flushPromises(): Promise { + for (let i = 0; i < 10; i++) await Promise.resolve(); +} + // ── Helpers ─────────────────────────────────────────────────────────────────── const emptyData: DashboardData = { @@ -108,12 +120,12 @@ describe("createPollCoordinator", () => { await createRoot(async (dispose) => { createPollCoordinator(makeGetInterval(60), fetchAll); - await Promise.resolve(); // initial fetch + await flushPromises(); // initial fetch // Advance 1 full interval (with jitter ±30s, 60s is within [30s, 90s]) // Use 90s to be safe and hit the interval regardless of jitter vi.advanceTimersByTime(90_000); - await Promise.resolve(); + await flushPromises(); expect(fetchAll.mock.calls.length).toBeGreaterThanOrEqual(2); dispose(); @@ -126,7 +138,7 @@ describe("createPollCoordinator", () => { await createRoot(async (dispose) => { createPollCoordinator(makeGetInterval(60), fetchAll); - await Promise.resolve(); // initial fetch + await flushPromises(); // initial fetch const callsAfterInit = fetchAll.mock.calls.length; @@ -135,7 +147,7 @@ describe("createPollCoordinator", () => { // Advance past the interval (60s with 0 jitter) vi.advanceTimersByTime(61_000); - await Promise.resolve(); + await flushPromises(); // Should have fetched while hidden (background refresh) expect(fetchAll.mock.calls.length).toBeGreaterThan(callsAfterInit); @@ -150,7 +162,7 @@ describe("createPollCoordinator", () => { await createRoot(async (dispose) => { createPollCoordinator(makeGetInterval(300), fetchAll); - await Promise.resolve(); // initial fetch + await flushPromises(); // initial fetch const callsAfterInit = fetchAll.mock.calls.length; @@ -162,7 +174,7 @@ describe("createPollCoordinator", () => { // Restore visibility setDocumentVisible(true); - await Promise.resolve(); + await flushPromises(); // Should have triggered at least a catch-up fetch on re-visible // (background polls may also have fired if interval < hidden duration) @@ -230,21 +242,21 @@ describe("createPollCoordinator", () => { await createRoot(async (dispose) => { createPollCoordinator(makeGetInterval(60), fetchAll); - await Promise.resolve(); // initial fetch + await flushPromises(); // initial fetch const callsAfterInit = fetchAll.mock.calls.length; // Hide for >2 min — background polls fire at 60s and 120s setDocumentVisible(false); vi.advanceTimersByTime(130_000); - await Promise.resolve(); + await flushPromises(); const callsWhileHidden = fetchAll.mock.calls.length; expect(callsWhileHidden).toBeGreaterThan(callsAfterInit); // Restore visibility — catch-up fetch fires + timer resets setDocumentVisible(true); - await Promise.resolve(); + await flushPromises(); const callsAfterRevisible = fetchAll.mock.calls.length; expect(callsAfterRevisible).toBeGreaterThan(callsWhileHidden); @@ -256,7 +268,7 @@ describe("createPollCoordinator", () => { // Advance another 31s (61s from reset) — timer fires vi.advanceTimersByTime(31_000); - await Promise.resolve(); + await flushPromises(); expect(fetchAll.mock.calls.length).toBeGreaterThan(callsAfterRevisible); dispose(); @@ -270,12 +282,12 @@ describe("createPollCoordinator", () => { await createRoot(async (dispose) => { const coordinator = createPollCoordinator(makeGetInterval(60), fetchAll); - await Promise.resolve(); // initial fetch + await flushPromises(); // initial fetch const callsAfterInit = fetchAll.mock.calls.length; coordinator.manualRefresh(); - await Promise.resolve(); + await flushPromises(); expect(fetchAll.mock.calls.length).toBe(callsAfterInit + 1); dispose(); @@ -353,6 +365,7 @@ describe("createPollCoordinator", () => { // During the in-flight fetch, isRefreshing should be true expect(coordinator.isRefreshing()).toBe(true); + await Promise.resolve(); // wait for fetchRateLimitDetails to resolve so fetchAll is called resolvePromise(); await Promise.resolve(); await Promise.resolve(); // allow finally block to run @@ -423,6 +436,7 @@ describe("createPollCoordinator", () => { // First fetch is in-flight (unresolved) expect(coordinator.isRefreshing()).toBe(true); + await Promise.resolve(); // wait for fetchRateLimitDetails so fetchAll is called expect(fetchAll).toHaveBeenCalledTimes(1); // Trigger a second fetch while the first is still in-flight @@ -599,13 +613,13 @@ describe("createPollCoordinator", () => { await createRoot(async (dispose) => { createPollCoordinator(makeGetInterval(60), fetchAll); - await Promise.resolve(); // initial fetch + await flushPromises(); // initial fetch const callsAfterInit = fetchAll.mock.calls.length; // Advance exactly past the deterministic 30s interval vi.advanceTimersByTime(30_001); - await Promise.resolve(); + await flushPromises(); expect(fetchAll.mock.calls.length).toBe(callsAfterInit + 1); dispose(); From 9cb01d5f5e642b7baf7322cd1ebe58149cd3c93d Mon Sep 17 00:00:00 2001 From: testvalue Date: Thu, 23 Apr 2026 08:42:15 -0400 Subject: [PATCH 3/7] 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 --- src/app/services/api.ts | 21 +++++++++-- tests/services/api.test.ts | 73 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 2 deletions(-) diff --git a/src/app/services/api.ts b/src/app/services/api.ts index 901baac4..74ff78c0 100644 --- a/src/app/services/api.ts +++ b/src/app/services/api.ts @@ -688,9 +688,13 @@ async function runForkPRFallback( if (pr) pr.checkStatus = mapCheckStatus(state); } } catch (err) { - const partialData = (err && typeof err === "object" && "data" in err && err.data && typeof err.data === "object") - ? err.data as Record + const errDataRaw = (err && typeof err === "object" && "data" in err && err.data && typeof err.data === "object") + ? err.data as Record : null; + if (errDataRaw?.rateLimit) { + updateGraphqlRateLimit(errDataRaw.rateLimit as GraphQLRateLimit); + } + const partialData = errDataRaw as Record | null; if (partialData) { for (let i = 0; i < forkChunk.length; i++) { @@ -1130,6 +1134,12 @@ export async function fetchPREnrichment( }); } } catch (err) { + const partialErr = (err && typeof err === "object" && "data" in err && err.data && typeof err.data === "object") + ? err.data as Partial + : null; + if (partialErr?.rateLimit) { + updateGraphqlRateLimit(partialErr.rateLimit); + } const { statusCode, message } = extractRejectionError(err); errors.push({ repo: `backfill-batch-${batchIdx + 1}/${batches.length}`, @@ -1686,6 +1696,13 @@ export async function fetchHotPRStatus( hadErrors = true; console.warn("[hot-poll] PR status batch failed:", s.reason); Sentry.captureException(s.reason, { tags: { source: "hot-poll-pr-batch" } }); + const reason = s.reason; + const partialErr = (reason && typeof reason === "object" && "data" in reason && reason.data && typeof reason.data === "object") + ? reason.data as Partial + : null; + if (partialErr?.rateLimit) { + updateGraphqlRateLimit(partialErr.rateLimit); + } } } diff --git a/tests/services/api.test.ts b/tests/services/api.test.ts index 4e99225b..05f3ee17 100644 --- a/tests/services/api.test.ts +++ b/tests/services/api.test.ts @@ -5,6 +5,8 @@ import { fetchRepos, fetchWorkflowRuns, validateGitHubUser, + fetchPREnrichment, + fetchHotPRStatus, type RepoRef, } from "../../src/app/services/api"; import { clearCache } from "../../src/app/stores/cache"; @@ -1137,3 +1139,74 @@ describe("fetchIssuesAndPullRequests — onLightData suppression when all monito expect(result.issues[0].id).toBe(5001); }); }); + +// ── Error-path rateLimit extraction calls updateGraphqlRateLimit ────────────── + +describe("error-path rateLimit extraction — updateGraphqlRateLimit called", () => { + const mockResetAt = "2026-04-23T12:00:00Z"; + + function makeGraphqlError(rateLimitOverride?: object) { + const rateLimit = rateLimitOverride ?? { limit: 5000, remaining: 100, resetAt: mockResetAt }; + return Object.assign(new Error("GraphQL partial error"), { + data: { rateLimit }, + }); + } + + beforeEach(() => { + vi.resetAllMocks(); + }); + + it("fetchPREnrichment error with partial rateLimit calls updateGraphqlRateLimit", async () => { + const githubModule = await import("../../src/app/services/github"); + const updateSpy = vi.spyOn(githubModule, "updateGraphqlRateLimit"); + + const octokit = makeOctokit( + async () => ({ data: {} }), + async () => { throw makeGraphqlError(); } + ); + + await fetchPREnrichment( + octokit as never, + ["PR_kwDOtest001"] + ); + + expect(updateSpy).toHaveBeenCalledWith( + expect.objectContaining({ limit: 5000, remaining: 100, resetAt: mockResetAt }) + ); + }); + + it("fetchHotPRStatus rejected batch with rateLimit calls updateGraphqlRateLimit", async () => { + const githubModule = await import("../../src/app/services/github"); + const updateSpy = vi.spyOn(githubModule, "updateGraphqlRateLimit"); + + const octokit = makeOctokit( + async () => ({ data: {} }), + async () => { throw makeGraphqlError(); } + ); + + await fetchHotPRStatus( + octokit as never, + ["PR_kwDOtest001"] + ); + + expect(updateSpy).toHaveBeenCalledWith( + expect.objectContaining({ limit: 5000, remaining: 100, resetAt: mockResetAt }) + ); + }); + + it("fetchPREnrichment second batch error with rateLimit calls updateGraphqlRateLimit", async () => { + const githubModule = await import("../../src/app/services/github"); + const updateSpy = vi.spyOn(githubModule, "updateGraphqlRateLimit"); + + const singleErrOctokit = makeOctokit( + async () => ({ data: {} }), + async () => { throw makeGraphqlError({ limit: 5000, remaining: 42, resetAt: mockResetAt }); } + ); + + await fetchPREnrichment(singleErrOctokit as never, ["PR_kwDOtest002"]); + + expect(updateSpy).toHaveBeenCalledWith( + expect.objectContaining({ limit: 5000, remaining: 42, resetAt: mockResetAt }) + ); + }); +}); From 44020dde7d0e97b1cdf15322d0d35ac4131beade Mon Sep 17 00:00:00 2001 From: testvalue Date: Thu, 23 Apr 2026 08:42:58 -0400 Subject: [PATCH 4/7] 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 --- .../components/dashboard/DashboardPage.tsx | 2 +- .../dashboard/DashboardPage.test.tsx | 87 +++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 tests/components/dashboard/DashboardPage.test.tsx diff --git a/src/app/components/dashboard/DashboardPage.tsx b/src/app/components/dashboard/DashboardPage.tsx index 035a2309..50130bb4 100644 --- a/src/app/components/dashboard/DashboardPage.tsx +++ b/src/app/components/dashboard/DashboardPage.tsx @@ -925,7 +925,7 @@ export default function DashboardPage() { {(rl) => (
- + API RL: {rl().remaining.toLocaleString()}/{formatCount(rl().limit)}/hr diff --git a/tests/components/dashboard/DashboardPage.test.tsx b/tests/components/dashboard/DashboardPage.test.tsx new file mode 100644 index 00000000..29f8aa5a --- /dev/null +++ b/tests/components/dashboard/DashboardPage.test.tsx @@ -0,0 +1,87 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { render, screen, cleanup } from "@solidjs/testing-library"; +import { Show } from "solid-js"; +import { updateGraphqlRateLimit, getGraphqlRateLimit } from "../../../src/app/services/github"; + +// Minimal component replicating the footer span CSS ternary from DashboardPage. +function RateLimitSpan() { + return ( + + {(rl) => ( + + API RL: {rl().remaining}/{rl().limit} + + )} + + ); +} + +afterEach(() => { + cleanup(); +}); + +// ── Footer span CSS class — three-tier logic ────────────────────────────────── + +describe("DashboardPage footer — rate limit span CSS classes", () => { + it("remaining: 0 gives text-error class", () => { + const resetAt = new Date(Date.now() + 3600 * 1000).toISOString(); + updateGraphqlRateLimit({ limit: 5000, remaining: 0, resetAt }); + + render(() => ); + const span = screen.getByTestId("rl-span"); + expect(span.className).toContain("text-error"); + expect(span.className).not.toContain("text-warning"); + }); + + it("remaining < 10% of limit gives text-warning class", () => { + const resetAt = new Date(Date.now() + 3600 * 1000).toISOString(); + // 100 / 5000 = 2% → below 10% threshold + updateGraphqlRateLimit({ limit: 5000, remaining: 100, resetAt }); + + render(() => ); + const span = screen.getByTestId("rl-span"); + expect(span.className).toContain("text-warning"); + expect(span.className).not.toContain("text-error"); + }); + + it("remaining >= 10% of limit gives neither CSS class", () => { + const resetAt = new Date(Date.now() + 3600 * 1000).toISOString(); + // 3000 / 5000 = 60% → normal range + updateGraphqlRateLimit({ limit: 5000, remaining: 3000, resetAt }); + + render(() => ); + const span = screen.getByTestId("rl-span"); + expect(span.className).not.toContain("text-error"); + expect(span.className).not.toContain("text-warning"); + }); + + it("remaining exactly at 10% threshold (= boundary) gives neither class", () => { + const resetAt = new Date(Date.now() + 3600 * 1000).toISOString(); + // 500 / 5000 = exactly 10% — NOT strictly less than, so no warning + updateGraphqlRateLimit({ limit: 5000, remaining: 500, resetAt }); + + render(() => ); + const span = screen.getByTestId("rl-span"); + expect(span.className).not.toContain("text-error"); + expect(span.className).not.toContain("text-warning"); + }); + + it("remaining just below 10% threshold gives text-warning", () => { + const resetAt = new Date(Date.now() + 3600 * 1000).toISOString(); + // 499 / 5000 = 9.98% → strictly less than 10% + updateGraphqlRateLimit({ limit: 5000, remaining: 499, resetAt }); + + render(() => ); + const span = screen.getByTestId("rl-span"); + expect(span.className).toContain("text-warning"); + }); +}); From 5159493c236b7c7e782a495a66fe09192d724577 Mon Sep 17 00:00:00 2001 From: testvalue Date: Thu, 23 Apr 2026 09:31:33 -0400 Subject: [PATCH 5/7] 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 --- src/app/services/github.ts | 4 +--- tests/services/poll.test.ts | 2 ++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/services/github.ts b/src/app/services/github.ts index c6d0c55f..da401a04 100644 --- a/src/app/services/github.ts +++ b/src/app/services/github.ts @@ -167,7 +167,7 @@ export function createGitHubClient(token: string): GitHubOctokitInstance { if (remaining !== undefined && reset !== undefined) { _setGraphqlRateLimit({ limit: safePositiveNumber(limit !== undefined ? parseInt(limit, 10) : NaN, _graphqlRateLimit()?.limit ?? 5000), - remaining: parseInt(remaining, 10), + remaining: Number.isFinite(parseInt(remaining, 10)) ? parseInt(remaining, 10) : 0, resetAt: new Date(parseInt(reset, 10) * 1000), }); } @@ -305,8 +305,6 @@ let _lastFetchResult: { core: RateLimitInfo; graphql: RateLimitInfo } | null = n export async function fetchRateLimitDetails(): Promise<{ core: RateLimitInfo; graphql: RateLimitInfo } | null> { // Return cached result within 5-second staleness window if (_lastFetchResult !== null && Date.now() - _lastFetchTime < 5000) { - _setCoreRateLimit(_lastFetchResult.core); - _setGraphqlRateLimit(_lastFetchResult.graphql); return { ..._lastFetchResult }; } diff --git a/tests/services/poll.test.ts b/tests/services/poll.test.ts index cd4155b1..8536bf8c 100644 --- a/tests/services/poll.test.ts +++ b/tests/services/poll.test.ts @@ -50,6 +50,8 @@ vi.mock("../../src/app/stores/config", () => ({ })); async function flushPromises(): Promise { + // doFetch() has multiple await points (fetchRateLimitDetails + fetchAll); + // 10 iterations ensures all chained microtasks settle regardless of depth. for (let i = 0; i < 10; i++) await Promise.resolve(); } From 5b57a0f3ac51b6fac63cf0490a2f81fe86de9b9b Mon Sep 17 00:00:00 2001 From: testvalue Date: Thu, 23 Apr 2026 11:24:30 -0400 Subject: [PATCH 6/7] 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 --- src/app/components/settings/SettingsPage.tsx | 4 +- src/app/services/api-usage.ts | 2 +- src/app/services/api.ts | 15 +++--- src/app/services/github.ts | 14 ++++- .../settings/ApiUsageSection.test.tsx | 8 +-- tests/services/github.test.ts | 54 +++++++++++++++++++ 6 files changed, 81 insertions(+), 16 deletions(-) diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index cf971985..a9a338dd 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -422,7 +422,7 @@ export default function SettingsPage() { Source Pool - Calls + Usage Last Called @@ -468,7 +468,7 @@ export default function SettingsPage() { onClick={() => resetUsageData()} class="btn btn-xs btn-ghost" > - Reset counts + Reset usage
diff --git a/src/app/services/api-usage.ts b/src/app/services/api-usage.ts index ab300fef..4c604779 100644 --- a/src/app/services/api-usage.ts +++ b/src/app/services/api-usage.ts @@ -224,7 +224,7 @@ export function deriveSource(info: ApiRequestInfo): ApiCallSource { onApiRequest((info) => { const source = deriveSource(info); const pool: ApiPool = info.isGraphql ? "graphql" : "core"; - trackApiCall(source, pool); + trackApiCall(source, pool, info.graphqlCost ?? 1); // Both pools tracked — Math.max keeps latest; may delay reset of the earlier pool's records if (info.resetEpochMs !== null) updateResetAt(info.resetEpochMs); }); diff --git a/src/app/services/api.ts b/src/app/services/api.ts index 74ff78c0..17eddc7c 100644 --- a/src/app/services/api.ts +++ b/src/app/services/api.ts @@ -12,6 +12,7 @@ export type { Issue, PullRequest, WorkflowRun, RepoRef, RepoEntry, OrgEntry, Che // ── Types ──────────────────────────────────────────────────────────────────── interface GraphQLRateLimit { + cost?: number; limit: number; remaining: number; resetAt: string; @@ -226,7 +227,7 @@ const ISSUES_SEARCH_QUERY = ` } } } - rateLimit { limit remaining resetAt } + rateLimit { cost limit remaining resetAt } } ${LIGHT_ISSUE_FRAGMENT} `; @@ -287,7 +288,7 @@ const LIGHT_COMBINED_SEARCH_QUERY = ` } } } - rateLimit { limit remaining resetAt } + rateLimit { cost limit remaining resetAt } } ${LIGHT_ISSUE_FRAGMENT} ${LIGHT_PR_FRAGMENT} @@ -318,7 +319,7 @@ const UNFILTERED_SEARCH_QUERY = ` } } } - rateLimit { limit remaining resetAt } + rateLimit { cost limit remaining resetAt } } ${LIGHT_ISSUE_FRAGMENT} ${LIGHT_PR_FRAGMENT} @@ -350,7 +351,7 @@ const LIGHT_PR_SEARCH_QUERY = ` } } } - rateLimit { limit remaining resetAt } + rateLimit { cost limit remaining resetAt } } ${LIGHT_PR_FRAGMENT} `; @@ -395,7 +396,7 @@ const HEAVY_PR_BACKFILL_QUERY = ` } } } - rateLimit { limit remaining resetAt } + rateLimit { cost limit remaining resetAt } } `; @@ -417,7 +418,7 @@ const HOT_PR_STATUS_QUERY = ` } } } - rateLimit { limit remaining resetAt } + rateLimit { cost limit remaining resetAt } } `; @@ -673,7 +674,7 @@ async function runForkPRFallback( ); } - const forkQuery = `query(${varDefs.join(", ")}) {\n${fragments.join("\n")}\nrateLimit { limit remaining resetAt }\n}`; + const forkQuery = `query(${varDefs.join(", ")}) {\n${fragments.join("\n")}\nrateLimit { cost limit remaining resetAt }\n}`; try { const forkResponse = await octokit.graphql(forkQuery, { ...variables, request: { apiSource: "forkCheck" } }); diff --git a/src/app/services/github.ts b/src/app/services/github.ts index da401a04..8975c96b 100644 --- a/src/app/services/github.ts +++ b/src/app/services/github.ts @@ -67,6 +67,8 @@ export interface ApiRequestInfo { apiSource?: string; /** x-ratelimit-reset converted to ms, or null if unavailable */ resetEpochMs: number | null; + /** GraphQL query point cost from response body, or undefined for REST */ + graphqlCost?: number; } const _requestCallbacks: Array<(info: ApiRequestInfo) => void> = []; @@ -152,8 +154,13 @@ export function createGitHubClient(token: string): GitHubOctokitInstance { let resetEpochMs: number | null = null; const errResetHeader = errResponse?.headers?.["x-ratelimit-reset"]; if (errResetHeader) resetEpochMs = parseInt(errResetHeader, 10) * 1000; + const errGraphqlCost = isGraphql + ? ((err as { data?: { rateLimit?: { cost?: number } } }).data?.rateLimit?.cost + ?? (errResponse as { data?: { data?: { rateLimit?: { cost?: number } } } } | undefined)?.data?.data?.rateLimit?.cost + ?? undefined) + : undefined; const info: ApiRequestInfo = { - url: options.url, method, status, isGraphql, apiSource, resetEpochMs, + url: options.url, method, status, isGraphql, apiSource, resetEpochMs, graphqlCost: errGraphqlCost, }; for (const cb of _requestCallbacks) { try { cb(info); } catch { /* swallow */ } } } @@ -183,8 +190,11 @@ export function createGitHubClient(token: string): GitHubOctokitInstance { const headers = (response.headers ?? {}) as Record; const resetHeader = headers["x-ratelimit-reset"]; const resetEpochMs = resetHeader ? parseInt(resetHeader, 10) * 1000 : null; + const graphqlCost = isGraphql + ? (response.data as { data?: { rateLimit?: { cost?: number } } })?.data?.rateLimit?.cost ?? undefined + : undefined; const info: ApiRequestInfo = { - url: options.url, method, status, isGraphql, apiSource, resetEpochMs, + url: options.url, method, status, isGraphql, apiSource, resetEpochMs, graphqlCost, }; for (const cb of _requestCallbacks) { try { cb(info); } catch { /* swallow */ } } diff --git a/tests/components/settings/ApiUsageSection.test.tsx b/tests/components/settings/ApiUsageSection.test.tsx index 45e9a79c..2a7e657f 100644 --- a/tests/components/settings/ApiUsageSection.test.tsx +++ b/tests/components/settings/ApiUsageSection.test.tsx @@ -274,18 +274,18 @@ describe("ApiUsageSection — reset button", () => { mockGetUsageResetAt.mockReturnValue(null); }); - it("renders the 'Reset counts' button", () => { + it("renders the 'Reset usage' button", () => { renderSettings(); - expect(screen.getByText("Reset counts")).toBeTruthy(); + expect(screen.getByText("Reset usage")).toBeTruthy(); }); - it("calls resetUsageData() when 'Reset counts' button is clicked", () => { + it("calls resetUsageData() when 'Reset usage' button is clicked", () => { // Wire the mock to clear snapshot on reset, simulating real behavior mockResetUsageData.mockImplementation(() => { mockGetUsageSnapshot.mockReturnValue([]); }); renderSettings(); - const btn = screen.getByText("Reset counts"); + const btn = screen.getByText("Reset usage"); fireEvent.click(btn); expect(mockResetUsageData).toHaveBeenCalledOnce(); }); diff --git a/tests/services/github.test.ts b/tests/services/github.test.ts index de9c3359..8d79b8ac 100644 --- a/tests/services/github.test.ts +++ b/tests/services/github.test.ts @@ -697,6 +697,60 @@ describe("hook.wrap — rate limit signal routing", () => { const graphqlRl = getGraphqlRateLimit(); expect(graphqlRl!.remaining).toBe(4321); }); + + it("successful GraphQL request exposes rateLimit.cost in ApiRequestInfo callback", async () => { + const resetEpoch = Math.floor(Date.now() / 1000) + 3600; + vi.stubGlobal("fetch", vi.fn().mockResolvedValue( + new Response(JSON.stringify({ + data: { + viewer: { login: "test" }, + rateLimit: { cost: 42, limit: 5000, remaining: 4900, resetAt: new Date(resetEpoch * 1000).toISOString() }, + }, + }), { + status: 200, + headers: { + "content-type": "application/json", + "x-ratelimit-remaining": "4900", + "x-ratelimit-reset": String(resetEpoch), + "x-ratelimit-limit": "5000", + }, + }) + )); + + let capturedInfo: ApiRequestInfo | null = null; + onApiRequest((info) => { capturedInfo = info; }); + + const client = createGitHubClient("test-token"); + await client.graphql("{ viewer { login } }"); + + expect(capturedInfo).not.toBeNull(); + expect(capturedInfo!.graphqlCost).toBe(42); + expect(capturedInfo!.isGraphql).toBe(true); + }); + + it("REST request has undefined graphqlCost in ApiRequestInfo callback", async () => { + vi.stubGlobal("fetch", vi.fn().mockResolvedValue( + new Response(JSON.stringify({ login: "test" }), { + status: 200, + headers: { + "content-type": "application/json", + "x-ratelimit-remaining": "4999", + "x-ratelimit-reset": String(Math.floor(Date.now() / 1000) + 3600), + "x-ratelimit-limit": "5000", + }, + }) + )); + + let capturedInfo: ApiRequestInfo | null = null; + onApiRequest((info) => { capturedInfo = info; }); + + const client = createGitHubClient("test-token"); + await client.request("GET /user"); + + expect(capturedInfo).not.toBeNull(); + expect(capturedInfo!.graphqlCost).toBeUndefined(); + expect(capturedInfo!.isGraphql).toBe(false); + }); }); // ── fetchRateLimitDetails ──────────────────────────────────────────────────── From a52126279f69469c41b5729740f9fc301454c33a Mon Sep 17 00:00:00 2001 From: testvalue Date: Thu, 23 Apr 2026 16:22:22 -0400 Subject: [PATCH 7/7] 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 --- .../components/dashboard/DashboardPage.tsx | 4 +- src/app/components/settings/SettingsPage.tsx | 12 +-- src/app/lib/format.ts | 6 ++ src/app/services/github.ts | 11 ++- src/app/services/poll.ts | 4 +- .../dashboard/DashboardPage.test.tsx | 87 +++---------------- tests/services/github.test.ts | 75 +++++++++++++++- tests/services/poll.test.ts | 24 ++++- 8 files changed, 130 insertions(+), 93 deletions(-) diff --git a/src/app/components/dashboard/DashboardPage.tsx b/src/app/components/dashboard/DashboardPage.tsx index 50130bb4..c7cfe55c 100644 --- a/src/app/components/dashboard/DashboardPage.tsx +++ b/src/app/components/dashboard/DashboardPage.tsx @@ -26,7 +26,7 @@ import { expireToken, user, onAuthCleared, DASHBOARD_STORAGE_KEY } from "../../s import { updateRelaySnapshot } from "../../lib/mcp-relay"; import { pushNotification } from "../../lib/errors"; import { getClient, getGraphqlRateLimit, fetchRateLimitDetails } from "../../services/github"; -import { formatCount, prSizeCategory } from "../../lib/format"; +import { formatCount, prSizeCategory, rateLimitCssClass } from "../../lib/format"; import { setsEqual } from "../../lib/collections"; import { withScrollLock } from "../../lib/scroll"; import { Tooltip } from "../shared/Tooltip"; @@ -925,7 +925,7 @@ export default function DashboardPage() { {(rl) => (
- + API RL: {rl().remaining.toLocaleString()}/{formatCount(rl().limit)}/hr diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index a9a338dd..7ff274ed 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -514,7 +514,7 @@ export default function SettingsPage() { - {/* Section 5: Notifications */} + {/* Section 6: Notifications */}
- {/* Section 6: Appearance */} + {/* Section 7: Appearance */}

Theme

@@ -634,7 +634,7 @@ export default function SettingsPage() {
- {/* Section 7: Tabs */} + {/* Section 8: Tabs */}
- {/* Section 8: Custom Tabs */} + {/* Section 9: Custom Tabs */}
r.owner))]} @@ -699,7 +699,7 @@ export default function SettingsPage() { />
- {/* Section 9: MCP Server Relay */} + {/* Section 10: MCP Server Relay */}
- {/* Section 10: Data */} + {/* Section 11: Data */}
{/* Authentication method */} { +export async function fetchRateLimitDetails(clientOverride?: GitHubOctokitInstance): Promise<{ core: RateLimitInfo; graphql: RateLimitInfo } | null> { // Return cached result within 5-second staleness window if (_lastFetchResult !== null && Date.now() - _lastFetchTime < 5000) { + _setCoreRateLimit(_lastFetchResult.core); + _setGraphqlRateLimit(_lastFetchResult.graphql); return { ..._lastFetchResult }; } - const client = getClient(); + const client = clientOverride ?? getClient(); if (!client) return null; try { diff --git a/src/app/services/poll.ts b/src/app/services/poll.ts index 850777e6..c42fa8de 100644 --- a/src/app/services/poll.ts +++ b/src/app/services/poll.ts @@ -357,7 +357,9 @@ export function createPollCoordinator( if (destroyed || isRefreshing()) return; checkAndResetIfExpired(); setIsRefreshing(true); - await fetchRateLimitDetails(); + // Fire-and-forget: seeds footer signals concurrently with fetchAll. If GET /rate_limit + // resolves after a GraphQL response, the footer briefly shows pre-query remaining (cosmetic). + void fetchRateLimitDetails(); // Snapshot sources of notifications from previous cycle (for reconciliation) const previousSources = new Set( diff --git a/tests/components/dashboard/DashboardPage.test.tsx b/tests/components/dashboard/DashboardPage.test.tsx index 29f8aa5a..cb44b82f 100644 --- a/tests/components/dashboard/DashboardPage.test.tsx +++ b/tests/components/dashboard/DashboardPage.test.tsx @@ -1,87 +1,24 @@ -import { describe, it, expect, afterEach } from "vitest"; -import { render, screen, cleanup } from "@solidjs/testing-library"; -import { Show } from "solid-js"; -import { updateGraphqlRateLimit, getGraphqlRateLimit } from "../../../src/app/services/github"; +import { describe, it, expect } from "vitest"; +import { rateLimitCssClass } from "../../../src/app/lib/format"; -// Minimal component replicating the footer span CSS ternary from DashboardPage. -function RateLimitSpan() { - return ( - - {(rl) => ( - - API RL: {rl().remaining}/{rl().limit} - - )} - - ); -} - -afterEach(() => { - cleanup(); -}); - -// ── Footer span CSS class — three-tier logic ────────────────────────────────── - -describe("DashboardPage footer — rate limit span CSS classes", () => { - it("remaining: 0 gives text-error class", () => { - const resetAt = new Date(Date.now() + 3600 * 1000).toISOString(); - updateGraphqlRateLimit({ limit: 5000, remaining: 0, resetAt }); - - render(() => ); - const span = screen.getByTestId("rl-span"); - expect(span.className).toContain("text-error"); - expect(span.className).not.toContain("text-warning"); +describe("rateLimitCssClass", () => { + it("remaining: 0 gives text-error", () => { + expect(rateLimitCssClass(0, 5000)).toBe("text-error"); }); - it("remaining < 10% of limit gives text-warning class", () => { - const resetAt = new Date(Date.now() + 3600 * 1000).toISOString(); - // 100 / 5000 = 2% → below 10% threshold - updateGraphqlRateLimit({ limit: 5000, remaining: 100, resetAt }); - - render(() => ); - const span = screen.getByTestId("rl-span"); - expect(span.className).toContain("text-warning"); - expect(span.className).not.toContain("text-error"); + it("remaining < 10% of limit gives text-warning", () => { + expect(rateLimitCssClass(100, 5000)).toBe("text-warning"); }); - it("remaining >= 10% of limit gives neither CSS class", () => { - const resetAt = new Date(Date.now() + 3600 * 1000).toISOString(); - // 3000 / 5000 = 60% → normal range - updateGraphqlRateLimit({ limit: 5000, remaining: 3000, resetAt }); - - render(() => ); - const span = screen.getByTestId("rl-span"); - expect(span.className).not.toContain("text-error"); - expect(span.className).not.toContain("text-warning"); + it("remaining >= 10% of limit gives empty string", () => { + expect(rateLimitCssClass(3000, 5000)).toBe(""); }); - it("remaining exactly at 10% threshold (= boundary) gives neither class", () => { - const resetAt = new Date(Date.now() + 3600 * 1000).toISOString(); - // 500 / 5000 = exactly 10% — NOT strictly less than, so no warning - updateGraphqlRateLimit({ limit: 5000, remaining: 500, resetAt }); - - render(() => ); - const span = screen.getByTestId("rl-span"); - expect(span.className).not.toContain("text-error"); - expect(span.className).not.toContain("text-warning"); + it("remaining exactly at 10% threshold gives empty string (strict less-than)", () => { + expect(rateLimitCssClass(500, 5000)).toBe(""); }); it("remaining just below 10% threshold gives text-warning", () => { - const resetAt = new Date(Date.now() + 3600 * 1000).toISOString(); - // 499 / 5000 = 9.98% → strictly less than 10% - updateGraphqlRateLimit({ limit: 5000, remaining: 499, resetAt }); - - render(() => ); - const span = screen.getByTestId("rl-span"); - expect(span.className).toContain("text-warning"); + expect(rateLimitCssClass(499, 5000)).toBe("text-warning"); }); }); diff --git a/tests/services/github.test.ts b/tests/services/github.test.ts index 8d79b8ac..c74d508a 100644 --- a/tests/services/github.test.ts +++ b/tests/services/github.test.ts @@ -751,6 +751,34 @@ describe("hook.wrap — rate limit signal routing", () => { expect(capturedInfo!.graphqlCost).toBeUndefined(); expect(capturedInfo!.isGraphql).toBe(false); }); + + it("GraphQL error response with rateLimit.cost in body exposes graphqlCost in ApiRequestInfo callback", async () => { + const resetEpoch = Math.floor(Date.now() / 1000) + 3600; + vi.stubGlobal("fetch", vi.fn().mockResolvedValue( + new Response(JSON.stringify({ + data: { rateLimit: { cost: 7, limit: 5000, remaining: 4800, resetAt: new Date(resetEpoch * 1000).toISOString() } }, + errors: [{ message: "resolver blew up" }], + }), { + status: 500, + headers: { + "content-type": "application/json", + "x-ratelimit-remaining": "4800", + "x-ratelimit-reset": String(resetEpoch), + "x-ratelimit-limit": "5000", + }, + }) + )); + + let capturedInfo: ApiRequestInfo | null = null; + onApiRequest((info) => { capturedInfo = info; }); + + const client = createGitHubClient("test-token"); + await expect(client.graphql("{ viewer { login } }", { request: { retries: 0 } })).rejects.toThrow(); + + expect(capturedInfo).not.toBeNull(); + expect(capturedInfo!.isGraphql).toBe(true); + expect(capturedInfo!.graphqlCost).toBe(7); + }); }); // ── fetchRateLimitDetails ──────────────────────────────────────────────────── @@ -758,7 +786,7 @@ describe("hook.wrap — rate limit signal routing", () => { // (_client) that is only non-null after auth token is set. These tests exercise // the null-client fast-path and the signal-preservation guarantee on failure. // Signal update tests (setCoreRateLimit / setGraphqlRateLimit) are covered by the -// hook.wrap tests above which exercise the same signal setters via real HTTP responses. +// hook.wrap tests above and the cache-hit path adds identical setters (same references). describe("fetchRateLimitDetails", () => { afterEach(() => { @@ -789,3 +817,48 @@ describe("fetchRateLimitDetails", () => { expect(getGraphqlRateLimit()!.remaining).toBe(2222); }); }); + +// ── fetchRateLimitDetails — client override + cache behavior ───────────────── + +describe("fetchRateLimitDetails — signal updates with client override", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("successful call updates both signals and cache-hit restores them", async () => { + const resetEpoch = Math.floor(Date.now() / 1000) + 3600; + const mockClient = { + request: vi.fn().mockResolvedValue({ + data: { + resources: { + core: { limit: 5000, remaining: 3000, reset: resetEpoch }, + graphql: { limit: 5000, remaining: 2500, reset: resetEpoch }, + }, + }, + }), + }; + + // Fresh call: seeds cache and updates signals + const result = await fetchRateLimitDetails(mockClient as never); + expect(result).not.toBeNull(); + expect(result!.core.remaining).toBe(3000); + expect(result!.graphql.remaining).toBe(2500); + expect(getCoreRateLimit()!.remaining).toBe(3000); + expect(getGraphqlRateLimit()!.remaining).toBe(2500); + + // Overwrite signals (simulates hook.wrap updating mid-cycle) + updateRateLimitFromHeaders({ + "x-ratelimit-remaining": "999", + "x-ratelimit-reset": String(resetEpoch), + "x-ratelimit-limit": "5000", + }); + expect(getCoreRateLimit()!.remaining).toBe(999); + + // Cache-hit within 5s: restores authoritative values, no HTTP call + const cached = await fetchRateLimitDetails(mockClient as never); + expect(cached).not.toBeNull(); + expect(getCoreRateLimit()!.remaining).toBe(3000); + expect(getGraphqlRateLimit()!.remaining).toBe(2500); + expect(mockClient.request).toHaveBeenCalledTimes(1); + }); +}); diff --git a/tests/services/poll.test.ts b/tests/services/poll.test.ts index 8536bf8c..8389485a 100644 --- a/tests/services/poll.test.ts +++ b/tests/services/poll.test.ts @@ -2,6 +2,7 @@ import "fake-indexeddb/auto"; import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { createRoot, createSignal } from "solid-js"; import { createPollCoordinator, disableNotifGate, resetPollState, type DashboardData } from "../../src/app/services/poll"; +import * as githubMod from "../../src/app/services/github"; // Mock pushError so we can spy on it const mockPushError = vi.fn(); @@ -32,7 +33,7 @@ vi.mock("../../src/app/lib/notifications", () => ({ _resetNotificationState: vi.fn(), })); -// Mock github module — fetchRateLimitDetails adds an async boundary in doFetch +// Mock github module — fetchRateLimitDetails runs concurrently (fire-and-forget) in doFetch vi.mock("../../src/app/services/github", () => ({ getClient: vi.fn(() => null), fetchRateLimitDetails: vi.fn(() => Promise.resolve(null)), @@ -50,7 +51,7 @@ vi.mock("../../src/app/stores/config", () => ({ })); async function flushPromises(): Promise { - // doFetch() has multiple await points (fetchRateLimitDetails + fetchAll); + // doFetch() awaits fetchAll and fires fetchRateLimitDetails concurrently; // 10 iterations ensures all chained microtasks settle regardless of depth. for (let i = 0; i < 10; i++) await Promise.resolve(); } @@ -367,7 +368,7 @@ describe("createPollCoordinator", () => { // During the in-flight fetch, isRefreshing should be true expect(coordinator.isRefreshing()).toBe(true); - await Promise.resolve(); // wait for fetchRateLimitDetails to resolve so fetchAll is called + await Promise.resolve(); // flush microtasks so fetchAll is called resolvePromise(); await Promise.resolve(); await Promise.resolve(); // allow finally block to run @@ -438,7 +439,7 @@ describe("createPollCoordinator", () => { // First fetch is in-flight (unresolved) expect(coordinator.isRefreshing()).toBe(true); - await Promise.resolve(); // wait for fetchRateLimitDetails so fetchAll is called + await Promise.resolve(); // flush microtasks so fetchAll is called expect(fetchAll).toHaveBeenCalledTimes(1); // Trigger a second fetch while the first is still in-flight @@ -661,4 +662,19 @@ describe("createPollCoordinator", () => { dispose(); }); }); + + it("fetchRateLimitDetails is called exactly once per doFetch cycle", async () => { + const fetchRateLimitDetailsSpy = vi.mocked(githubMod.fetchRateLimitDetails); + fetchRateLimitDetailsSpy.mockClear(); + + const fetchAll = makeFetchAll(); + + await createRoot(async (dispose) => { + createPollCoordinator(makeGetInterval(0), fetchAll); + await flushPromises(); + + expect(fetchRateLimitDetailsSpy).toHaveBeenCalledTimes(1); + dispose(); + }); + }); });