fix(api-usage): addresses PR review findings

- Derives ApiCallSource/ApiPool types from const arrays (single source of truth)
- Uses z.enum() in Zod schemas, removing unsafe type cast
- Validates apiSource in deriveSource to prevent invalid strings reaching storage
- Moves SOURCE_LABELS to api-usage.ts with Record<ApiCallSource, string> exhaustiveness
- Inlines checkAndResetIfExpired to single localStorage write
- Removes _requestCallbacks.length guard (always populated by api-usage.ts)
- Includes REST reset times in updateResetAt via Math.max
- Replaces positional callback capture with invoke-all-callbacks pattern
- Adds tests for guard conditions, multi-client callbacks, read-only guard,
  quota errors, null client path, and hermetic staleness cache

Author: wgordon17

src/app/components/settings/SettingsPage.tsx

@@ -9,7 +9,7 @@ import { isSafeGitHubUrl, openGitHubUrl } from "../../lib/url";
 import { relativeTime } from "../../lib/format";
 import { fetchOrgs } from "../../services/api";
 import { getClient } from "../../services/github";
-import { getUsageSnapshot, getUsageResetAt, resetUsageData, checkAndResetIfExpired } from "../../services/api-usage";
+import { getUsageSnapshot, getUsageResetAt, resetUsageData, checkAndResetIfExpired, SOURCE_LABELS } from "../../services/api-usage";
 import OrgSelector from "../onboarding/OrgSelector";
 import RepoSelector from "../onboarding/RepoSelector";
 import Section from "./Section";
@@ -19,25 +19,6 @@ import TrackedUsersSection from "./TrackedUsersSection";
 import { InfoTooltip } from "../shared/Tooltip";
 import type { RepoRef } from "../../services/api";
 
-const SOURCE_LABELS: Record<string, string> = {
-  lightSearch: "Light Search",
-  heavyBackfill: "PR Backfill",
-  forkCheck: "Fork Check",
-  globalUserSearch: "Tracked User Search",
-  unfilteredSearch: "Unfiltered Search",
-  upstreamDiscovery: "Upstream Discovery",
-  workflowRuns: "Workflow Runs",
-  hotPRStatus: "Hot PR Status",
-  hotRunStatus: "Hot Run Status",
-  notifications: "Notifications",
-  validateUser: "Validate User",
-  fetchOrgs: "Fetch Orgs",
-  fetchRepos: "Fetch Repos",
-  rateLimitCheck: "Rate Limit Check",
-  graphql: "GraphQL (other)",
-  rest: "REST (other)",
-};
-
 export default function SettingsPage() {
   const navigate = useNavigate();
 

src/app/components/shared/Tooltip.tsx

@@ -61,7 +61,7 @@ export function Tooltip(props: TooltipProps) {
         {props.children}
       </KobalteTooltip.Trigger>
       <KobalteTooltip.Portal>
-        <KobalteTooltip.Content class={TOOLTIP_CONTENT_CLASS + (props.contentClass ? ` ${props.contentClass}` : "")}>
+        <KobalteTooltip.Content class={`${TOOLTIP_CONTENT_CLASS}${props.contentClass ? ` ${props.contentClass}` : ""}`}>
           <KobalteTooltip.Arrow />
           {props.content}
         </KobalteTooltip.Content>

src/app/services/api-usage.ts

@@ -6,25 +6,36 @@ import { onApiRequest, type ApiRequestInfo } from "./github";
 
 // ── Types ─────────────────────────────────────────────────────────────────────
 
-export type ApiCallSource =
-  | "lightSearch"
-  | "heavyBackfill"
-  | "forkCheck"
-  | "globalUserSearch"
-  | "unfilteredSearch"
-  | "upstreamDiscovery"
-  | "workflowRuns"
-  | "hotPRStatus"
-  | "hotRunStatus"
-  | "notifications"
-  | "validateUser"
-  | "fetchOrgs"
-  | "fetchRepos"
-  | "rateLimitCheck"
-  | "graphql"
-  | "rest";
-
-export type ApiPool = "graphql" | "core";
+const API_CALL_SOURCES = [
+  "lightSearch", "heavyBackfill", "forkCheck", "globalUserSearch", "unfilteredSearch",
+  "upstreamDiscovery", "workflowRuns", "hotPRStatus", "hotRunStatus", "notifications",
+  "validateUser", "fetchOrgs", "fetchRepos", "rateLimitCheck", "graphql", "rest",
+] as const;
+
+export type ApiCallSource = (typeof API_CALL_SOURCES)[number];
+
+const API_POOLS = ["graphql", "core"] as const;
+
+export type ApiPool = (typeof API_POOLS)[number];
+
+export const SOURCE_LABELS: Record<ApiCallSource, string> = {
+  lightSearch: "Light Search",
+  heavyBackfill: "PR Backfill",
+  forkCheck: "Fork Check",
+  globalUserSearch: "Tracked User Search",
+  unfilteredSearch: "Unfiltered Search",
+  upstreamDiscovery: "Upstream Discovery",
+  workflowRuns: "Workflow Runs",
+  hotPRStatus: "Hot PR Status",
+  hotRunStatus: "Hot Run Status",
+  notifications: "Notifications",
+  validateUser: "Validate User",
+  fetchOrgs: "Fetch Orgs",
+  fetchRepos: "Fetch Repos",
+  rateLimitCheck: "Rate Limit Check",
+  graphql: "GraphQL (other)",
+  rest: "REST (other)",
+};
 
 export interface ApiCallRecord {
   source: ApiCallSource;
@@ -45,8 +56,8 @@ const USAGE_STORAGE_KEY = "github-tracker:api-usage";
 // ── Zod schemas ───────────────────────────────────────────────────────────────
 
 const ApiCallRecordSchema = z.object({
-  source: z.string(),
-  pool: z.string(),
+  source: z.enum(API_CALL_SOURCES),
+  pool: z.enum(API_POOLS),
   count: z.number(),
   lastCalledAt: z.number(),
 });
@@ -64,7 +75,7 @@ export function loadUsageData(): ApiUsageData {
     if (raw === null || raw === undefined) return { records: {}, resetAt: null };
     const parsed = JSON.parse(raw) as unknown;
     const result = ApiUsageDataSchema.safeParse(parsed);
-    if (result.success) return result.data as ApiUsageData;
+    if (result.success) return result.data;
     return { records: {}, resetAt: null };
   } catch {
     return { records: {}, resetAt: null };
@@ -104,10 +115,10 @@ export function clearUsageData(): void {
 
 export function checkAndResetIfExpired(): void {
   if (_usageData.resetAt !== null && Date.now() > _usageData.resetAt) {
-    resetUsageData();
+    _usageData.records = {};
     _usageData.resetAt = null;
-    // Write immediately so the null resetAt persists (prevents redundant re-reset on page reload)
     _writeToStorage();
+    _setVersion((v) => v + 1);
   }
 }
 
@@ -195,9 +206,13 @@ const REST_SOURCE_PATTERNS: Array<[RegExp, ApiCallSource]> = [
   [/^\/rate_limit/, "rateLimitCheck"],
 ];
 
+const API_CALL_SOURCE_SET = new Set<string>(API_CALL_SOURCES);
+
 export function deriveSource(info: ApiRequestInfo): ApiCallSource {
   if (info.isGraphql) {
-    return (info.apiSource as ApiCallSource) ?? "graphql";
+    return info.apiSource && API_CALL_SOURCE_SET.has(info.apiSource)
+      ? (info.apiSource as ApiCallSource)
+      : "graphql";
   }
   for (const [pattern, source] of REST_SOURCE_PATTERNS) {
     if (pattern.test(info.url)) return source;
@@ -210,5 +225,6 @@ onApiRequest((info) => {
   const source = deriveSource(info);
   const pool: ApiPool = info.isGraphql ? "graphql" : "core";
   trackApiCall(source, pool);
-  if (info.resetEpochMs !== null && info.isGraphql) updateResetAt(info.resetEpochMs);
+  // Both pools tracked — Math.max keeps latest; may delay reset of the earlier pool's records
+  if (info.resetEpochMs !== null) updateResetAt(info.resetEpochMs);
 });

src/app/services/github.ts

@@ -151,7 +151,7 @@ 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.
-      if (status > 0 && _requestCallbacks.length > 0) {
+      if (status > 0) {
         let resetEpochMs: number | null = null;
         const errResponse = (err as { response?: { headers?: Record<string, string> } }).response;
         const errResetHeader = errResponse?.headers?.["x-ratelimit-reset"];
@@ -164,16 +164,14 @@ export function createGitHubClient(token: string): GitHubOctokitInstance {
       throw err;
     }
 
-    // Success path — fire callbacks and update RL display
-    if (_requestCallbacks.length > 0) {
-      const headers = (response.headers ?? {}) as Record<string, string>;
-      const resetHeader = headers["x-ratelimit-reset"];
-      const resetEpochMs = resetHeader ? parseInt(resetHeader, 10) * 1000 : null;
-      const info: ApiRequestInfo = {
-        url: options.url, method, status, isGraphql, apiSource, resetEpochMs,
-      };
-      for (const cb of _requestCallbacks) { try { cb(info); } catch { /* swallow */ } }
-    }
+    // Success path — fire callbacks (api-usage.ts registers at module scope) and update RL display
+    const headers = (response.headers ?? {}) as Record<string, string>;
+    const resetHeader = headers["x-ratelimit-reset"];
+    const resetEpochMs = resetHeader ? parseInt(resetHeader, 10) * 1000 : null;
+    const info: ApiRequestInfo = {
+      url: options.url, method, status, isGraphql, apiSource, resetEpochMs,
+    };
+    for (const cb of _requestCallbacks) { try { cb(info); } catch { /* swallow */ } }
 
     if (response.headers) {
       updateRateLimitFromHeaders(response.headers as Record<string, string>);

tests/components/settings/ApiUsageSection.test.tsx

@@ -31,7 +31,14 @@ vi.mock("../../../src/app/services/api-usage", () => ({
   getUsageSnapshot: () => mockGetUsageSnapshot(),
   getUsageResetAt: () => mockGetUsageResetAt(),
   resetUsageData: () => mockResetUsageData(),
-  // Stubs for module-level side effects in dependent modules
+  SOURCE_LABELS: {
+    lightSearch: "Light Search", heavyBackfill: "PR Backfill", forkCheck: "Fork Check",
+    globalUserSearch: "Tracked User Search", unfilteredSearch: "Unfiltered Search",
+    upstreamDiscovery: "Upstream Discovery", workflowRuns: "Workflow Runs",
+    hotPRStatus: "Hot PR Status", hotRunStatus: "Hot Run Status", notifications: "Notifications",
+    validateUser: "Validate User", fetchOrgs: "Fetch Orgs", fetchRepos: "Fetch Repos",
+    rateLimitCheck: "Rate Limit Check", graphql: "GraphQL (other)", rest: "REST (other)",
+  },
   trackApiCall: vi.fn(),
   updateResetAt: vi.fn(),
   checkAndResetIfExpired: vi.fn(),
@@ -273,6 +280,10 @@ describe("ApiUsageSection — reset button", () => {
   });
 
   it("calls resetUsageData() when 'Reset counts' 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");
     fireEvent.click(btn);

tests/services/api-usage.test.ts

@@ -187,6 +187,18 @@ describe("flushUsageData / loadUsageData", () => {
     expect(freshMod.getUsageSnapshot()).toHaveLength(0);
   });
 
+  it("calls pushNotification when localStorage.setItem throws (quota exceeded)", async () => {
+    const { pushNotification } = await import("../../src/app/lib/errors");
+    vi.spyOn(localStorageMock, "setItem").mockImplementationOnce(() => { throw new DOMException("QuotaExceededError"); });
+    mod.trackApiCall("lightSearch", "graphql");
+    vi.advanceTimersByTime(600); // fire debounced flush
+    expect(vi.mocked(pushNotification)).toHaveBeenCalledWith(
+      "localStorage:api-usage",
+      expect.stringContaining("write failed"),
+      "warning"
+    );
+  });
+
   it("loadUsageData restores state on module init from valid localStorage", async () => {
     const storedData = {
       records: {
@@ -342,6 +354,29 @@ describe("checkAndResetIfExpired", () => {
     const parsed = JSON.parse(raw!);
     expect(parsed.resetAt).toBeNull();
   });
+
+  it("clears records and nulls resetAt atomically on expiry", () => {
+    vi.setSystemTime(new Date("2026-01-01T12:00:00Z"));
+    mod.trackApiCall("lightSearch", "graphql");
+    mod.updateResetAt(new Date("2026-01-01T11:00:00Z").getTime());
+    // Flush pending writes so state is settled
+    vi.advanceTimersByTime(600);
+
+    // Record localStorage state before the expiry check
+    const before = localStorageMock.getItem("github-tracker:api-usage");
+    const beforeParsed = JSON.parse(before!);
+    expect(beforeParsed.resetAt).not.toBeNull();
+    expect(Object.keys(beforeParsed.records)).toHaveLength(1);
+
+    vi.setSystemTime(new Date("2026-01-01T12:01:00Z"));
+    mod.checkAndResetIfExpired();
+
+    // After expiry: records cleared, resetAt null — written in a single pass
+    const after = localStorageMock.getItem("github-tracker:api-usage");
+    const afterParsed = JSON.parse(after!);
+    expect(afterParsed.resetAt).toBeNull();
+    expect(Object.keys(afterParsed.records)).toHaveLength(0);
+  });
 });
 
 describe("updateResetAt", () => {
@@ -369,6 +404,17 @@ describe("updateResetAt", () => {
     mod.updateResetAt(5000);
     expect(mod.getUsageResetAt()).toBe(9000);
   });
+
+  it.each([0, -1, NaN, Infinity, -Infinity])("rejects invalid resetAt: %s", (val) => {
+    mod.updateResetAt(val);
+    expect(mod.getUsageResetAt()).toBeNull();
+  });
+
+  it("does not overwrite valid resetAt with invalid value", () => {
+    mod.updateResetAt(5000);
+    mod.updateResetAt(NaN);
+    expect(mod.getUsageResetAt()).toBe(5000);
+  });
 });
 
 // ── deriveSource ─────────────────────────────────────────────────────────────
@@ -423,4 +469,8 @@ describe("deriveSource — URL pattern matching", () => {
   it("hotRunStatus pattern takes priority over workflowRuns (specific before general)", () => {
     expect(mod.deriveSource(makeInfo({ url: "/repos/foo/bar/actions/runs/999" }))).toBe("hotRunStatus");
   });
+
+  it("falls back to 'graphql' for unrecognized apiSource string", () => {
+    expect(mod.deriveSource(makeInfo({ isGraphql: true, url: "/graphql", apiSource: "unknownLabel" }))).toBe("graphql");
+  });
 });

tests/services/github-rate-limit.test.ts

@@ -61,57 +61,65 @@ describe("fetchRateLimitDetails — success response", () => {
     expect(result!.graphql.resetAt).toBeInstanceOf(Date);
   });
 
-  it("does not cache failures — returns null on error response without caching", async () => {
-    // First call with error — should return null
-    vi.stubGlobal("fetch", vi.fn().mockResolvedValue(makeSuccessResponse())); // seed cache
-    await fetchRateLimitDetails();
-
-    // Now force a network error: since result is cached (within 5s), it returns cached
-    // We cannot easily test failure with caching without fake timers.
-    // Instead, test that the function handles errors gracefully at all.
-    // This is tested via: mock a response that results in no graphql key
+  it("returns null when response body is missing the graphql key", async () => {
+    // Use fresh module to bypass staleness cache
+    vi.resetModules();
+    vi.doMock("../../src/app/stores/auth", () => ({
+      token: () => "fake-token-for-rate-limit-tests",
+      onAuthCleared: vi.fn(),
+    }));
     vi.stubGlobal("fetch", vi.fn().mockResolvedValue(
       new Response(
         JSON.stringify({ resources: { core: { limit: 5000, remaining: 4800, reset: 99999, used: 0 } } }),
         { status: 200, headers: { "content-type": "application/json" } }
       )
     ));
 
-    // The cache from the prior test means this won't re-fetch within 5s
-    // That's expected behavior — just verify cached result is returned
-    const result = await fetchRateLimitDetails();
-    // Either the cache hit (from prior test within 5s) or a new fetch succeeded
-    // In both cases, result should not be null
-    expect(result).not.toBeNull();
+    const { fetchRateLimitDetails: freshFetch } = await import("../../src/app/services/github");
+    const result = await freshFetch();
+    expect(result).toBeNull();
   });
 });
 
 describe("fetchRateLimitDetails — staleness cache", () => {
-  afterEach(() => {
-    vi.unstubAllGlobals();
-  });
-
   it("returns the same data for two calls within 5 seconds (cache hit)", async () => {
+    // Fresh module for hermetic test — no cross-test cache dependency
+    vi.resetModules();
+    vi.doMock("../../src/app/stores/auth", () => ({
+      token: () => "fake-token-for-staleness-test",
+      onAuthCleared: vi.fn(),
+    }));
     const mockFetch = vi.fn().mockResolvedValue(makeSuccessResponse());
     vi.stubGlobal("fetch", mockFetch);
 
-    // First call — may hit cache from prior test or hit network
-    const result1 = await fetchRateLimitDetails();
+    const { fetchRateLimitDetails: freshFetch } = await import("../../src/app/services/github");
+
+    const result1 = await freshFetch();
     expect(result1).not.toBeNull();
-    const callsAfterFirst = mockFetch.mock.calls.length;
+    expect(mockFetch).toHaveBeenCalledTimes(1);
 
     // Second call immediately — must not make a new network request
-    const result2 = await fetchRateLimitDetails();
+    const result2 = await freshFetch();
     expect(result2).not.toBeNull();
-    expect(mockFetch.mock.calls.length).toBe(callsAfterFirst); // no extra calls
-    expect(result2!.core.remaining).toBe(result1!.core.remaining); // same data
+    expect(mockFetch).toHaveBeenCalledTimes(1); // no extra calls
+    expect(result2!.core.remaining).toBe(result1!.core.remaining);
+
+    vi.unstubAllGlobals();
   });
 });
 
 describe("fetchRateLimitDetails — null client", () => {
-  // Cannot reliably test null-client path due to module-level staleness cache.
-  // getClient() is only called when cache is expired (>5s), and vi.resetModules()
-  // + dynamic import is needed for clean state — but that conflicts with the auth
-  // module's eager client creation. Documented as a known test limitation.
-  it.todo("returns null when getClient() returns null");
+  it("returns null when getClient() returns null", async () => {
+    // Use vi.resetModules() for clean module state (clears staleness cache).
+    // Mock auth to return null token so no eager client is created.
+    vi.resetModules();
+    vi.doMock("../../src/app/stores/auth", () => ({
+      token: () => null,
+      onAuthCleared: vi.fn(),
+    }));
+
+    const { fetchRateLimitDetails: freshFetch } = await import("../../src/app/services/github");
+    const result = await freshFetch();
+    expect(result).toBeNull();
+  });
 });