From dcb022d0217619f6f13f0eb15bf3d437b5757755 Mon Sep 17 00:00:00 2001 From: testvalue Date: Mon, 30 Mar 2026 10:44:44 -0400 Subject: [PATCH 1/4] fix(config): prevents settings reset on partial update --- src/app/stores/config.ts | 10 +++++-- tests/stores/config.test.ts | 58 +++++++++++++++++++++++++++++++++---- 2 files changed, 60 insertions(+), 8 deletions(-) diff --git a/src/app/stores/config.ts b/src/app/stores/config.ts index c1a5a0e4..9802aa74 100644 --- a/src/app/stores/config.ts +++ b/src/app/stores/config.ts @@ -72,10 +72,16 @@ export const [config, setConfig] = createStore(loadConfig()); export function updateConfig(partial: Partial): void { const validated = ConfigSchema.partial().safeParse(partial); - if (!validated.success) return; // reject invalid updates + if (!validated.success) return; + // Only merge keys the caller actually provided. ConfigSchema.partial() marks + // all fields optional but Zod still applies per-field .default() values for + // absent keys, so validated.data contains defaults for every omitted field. + // Filtering to keysProvided prevents those defaults from overwriting live state. + const keysProvided = Object.keys(partial) as (keyof Config)[]; + const filtered = Object.fromEntries(keysProvided.map((k) => [k, validated.data[k]])); setConfig( produce((draft) => { - Object.assign(draft, validated.data); + Object.assign(draft, filtered); }) ); } diff --git a/tests/stores/config.test.ts b/tests/stores/config.test.ts index 392456d3..bbd67793 100644 --- a/tests/stores/config.test.ts +++ b/tests/stores/config.test.ts @@ -143,8 +143,9 @@ describe("loadConfig", () => { }); }); -// Each updateConfig test creates its own isolated store to avoid shared state -describe("updateConfig", () => { +// Tests SolidJS store merge mechanics in isolation (does NOT exercise the real +// updateConfig export — see "updateConfig (real export)" block below for that). +describe("store merge behavior (produce + Object.assign)", () => { function makeStore() { const [cfg, setCfg] = createStore(ConfigSchema.parse({})); const update = (partial: Partial>) => { @@ -214,10 +215,7 @@ describe("updateConfig", () => { describe("updateConfig (real export)", () => { beforeEach(() => { - createRoot((dispose) => { - resetConfig(); - dispose(); - }); + resetConfig(); }); it("applies valid partial updates", () => { @@ -243,4 +241,52 @@ describe("updateConfig (real export)", () => { dispose(); }); }); + + it("preserves non-default values when updating a different field", () => { + createRoot((dispose) => { + // Set several fields to non-default values + updateConfig({ theme: "dark", refreshInterval: 120, itemsPerPage: 50 }); + // Now update a single unrelated field + updateConfig({ hotPollInterval: 60 }); + // All previously-set fields must survive + expect(config.hotPollInterval).toBe(60); + expect(config.theme).toBe("dark"); + expect(config.refreshInterval).toBe(120); + expect(config.itemsPerPage).toBe(50); + dispose(); + }); + }); + + it("preserves onboardingComplete when updating refreshInterval", () => { + createRoot((dispose) => { + updateConfig({ onboardingComplete: true }); + updateConfig({ refreshInterval: 600 }); + expect(config.refreshInterval).toBe(600); + expect(config.onboardingComplete).toBe(true); + dispose(); + }); + }); + + it("preserves nested notifications when updating a top-level field", () => { + createRoot((dispose) => { + updateConfig({ + notifications: { enabled: true, issues: true, pullRequests: false, workflowRuns: true }, + }); + updateConfig({ viewDensity: "compact" }); + expect(config.viewDensity).toBe("compact"); + expect(config.notifications.enabled).toBe(true); + expect(config.notifications.pullRequests).toBe(false); + dispose(); + }); + }); + + it("preserves selectedOrgs when updating theme", () => { + createRoot((dispose) => { + updateConfig({ selectedOrgs: ["my-org", "other-org"] }); + updateConfig({ theme: "forest" }); + expect(config.theme).toBe("forest"); + expect(config.selectedOrgs).toEqual(["my-org", "other-org"]); + dispose(); + }); + }); }); From f019549c5aaa3f8b2216a8263f6eb2c1c9bff6e4 Mon Sep 17 00:00:00 2001 From: testvalue Date: Mon, 30 Mar 2026 11:02:00 -0400 Subject: [PATCH 2/4] fix(config): addresses PR review findings - merges split solid-js/store imports in test file - trims redundant comment line in updateConfig - adds empty-object regression test for updateConfig - adds mixed valid/invalid field rejection test --- src/app/stores/config.ts | 7 +++---- tests/stores/config.test.ts | 22 ++++++++++++++++++++-- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/app/stores/config.ts b/src/app/stores/config.ts index 9802aa74..7cdd6a04 100644 --- a/src/app/stores/config.ts +++ b/src/app/stores/config.ts @@ -73,10 +73,9 @@ export const [config, setConfig] = createStore(loadConfig()); export function updateConfig(partial: Partial): void { const validated = ConfigSchema.partial().safeParse(partial); if (!validated.success) return; - // Only merge keys the caller actually provided. ConfigSchema.partial() marks - // all fields optional but Zod still applies per-field .default() values for - // absent keys, so validated.data contains defaults for every omitted field. - // Filtering to keysProvided prevents those defaults from overwriting live state. + // Only merge keys the caller actually provided: Zod .partial().safeParse() + // still applies per-field .default() values for absent keys, inflating + // validated.data with defaults that would overwrite live state. const keysProvided = Object.keys(partial) as (keyof Config)[]; const filtered = Object.fromEntries(keysProvided.map((k) => [k, validated.data[k]])); setConfig( diff --git a/tests/stores/config.test.ts b/tests/stores/config.test.ts index bbd67793..232767bf 100644 --- a/tests/stores/config.test.ts +++ b/tests/stores/config.test.ts @@ -1,8 +1,7 @@ import { describe, it, expect, beforeEach } from "vitest"; import { ConfigSchema, loadConfig, config, updateConfig, resetConfig } from "../../src/app/stores/config"; import { createRoot } from "solid-js"; -import { createStore } from "solid-js/store"; -import { produce } from "solid-js/store"; +import { createStore, produce } from "solid-js/store"; // Mock localStorage const localStorageMock = (() => { @@ -289,4 +288,23 @@ describe("updateConfig (real export)", () => { dispose(); }); }); + + it("does nothing when called with empty object", () => { + createRoot((dispose) => { + updateConfig({ theme: "dark" }); + updateConfig({}); + expect(config.theme).toBe("dark"); + dispose(); + }); + }); + + it("rejects entire update when any field is invalid", () => { + createRoot((dispose) => { + updateConfig({ theme: "dark" }); + updateConfig({ theme: "forest", hotPollInterval: 5 }); // hotPollInterval below min + expect(config.theme).toBe("dark"); + expect(config.hotPollInterval).toBe(30); + dispose(); + }); + }); }); From f8cdb7c38f17eef450690099b31245b1e807c9c8 Mon Sep 17 00:00:00 2001 From: testvalue Date: Mon, 30 Mar 2026 11:08:10 -0400 Subject: [PATCH 3/4] fix(api): uses dynamic rate limit instead of hardcoded 5k Queries limit field from GraphQL rateLimit object and displays it dynamically in the footer. GitHub Enterprise Cloud orgs grant 10k pts/hr instead of the standard 5k, causing the old hardcoded display to show impossible values like 9000/5k/hr. Warning threshold is now proportional (< limit * 0.1) instead of hardcoded < 500. Adds safePositiveInt guard to reject 0/NaN/Infinity limit values with fallback to previous or 5000. --- e2e/settings.spec.ts | 2 +- e2e/smoke.spec.ts | 4 +- .../components/dashboard/DashboardPage.tsx | 5 ++- src/app/services/api.ts | 38 +++++++++--------- src/app/services/github.ts | 11 ++++- tests/integration/data-pipeline.test.ts | 2 +- tests/services/api-optimization.test.ts | 2 +- tests/services/api.test.ts | 18 ++++----- tests/services/github.test.ts | 15 +++++-- tests/services/hot-poll.test.ts | 40 +++++++++---------- 10 files changed, 78 insertions(+), 59 deletions(-) diff --git a/e2e/settings.spec.ts b/e2e/settings.spec.ts index d0f1c92f..31e8a4ed 100644 --- a/e2e/settings.spec.ts +++ b/e2e/settings.spec.ts @@ -25,7 +25,7 @@ async function setupAuth(page: Page) { json: { data: { search: { issueCount: 0, pageInfo: { hasNextPage: false, endCursor: null }, nodes: [] }, - rateLimit: { remaining: 5000, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 5000, resetAt: new Date(Date.now() + 3600000).toISOString() }, }, }, }) diff --git a/e2e/smoke.spec.ts b/e2e/smoke.spec.ts index 5603e4ee..92aaaa75 100644 --- a/e2e/smoke.spec.ts +++ b/e2e/smoke.spec.ts @@ -34,7 +34,7 @@ async function setupAuth(page: Page) { json: { data: { search: { issueCount: 0, pageInfo: { hasNextPage: false, endCursor: null }, nodes: [] }, - rateLimit: { remaining: 5000, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 5000, resetAt: new Date(Date.now() + 3600000).toISOString() }, }, }, }) @@ -112,7 +112,7 @@ test("OAuth callback flow completes and redirects", async ({ page }) => { json: { data: { search: { issueCount: 0, pageInfo: { hasNextPage: false, endCursor: null }, nodes: [] }, - rateLimit: { remaining: 5000, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 5000, resetAt: new Date(Date.now() + 3600000).toISOString() }, }, }, }) diff --git a/src/app/components/dashboard/DashboardPage.tsx b/src/app/components/dashboard/DashboardPage.tsx index 3430f48a..2f01a730 100644 --- a/src/app/components/dashboard/DashboardPage.tsx +++ b/src/app/components/dashboard/DashboardPage.tsx @@ -21,6 +21,7 @@ import { } from "../../services/poll"; import { clearAuth, user, onAuthCleared, DASHBOARD_STORAGE_KEY } from "../../stores/auth"; import { getClient, getGraphqlRateLimit } from "../../services/github"; +import { formatCount } from "../../lib/format"; // ── Shared dashboard store (module-level to survive navigation) ───────────── @@ -377,8 +378,8 @@ export default function DashboardPage() { {(rl) => (
- - API RL: {rl().remaining.toLocaleString()}/5k/hr + + API RL: {rl().remaining.toLocaleString()}/{formatCount(rl().limit)}/hr
)} diff --git a/src/app/services/api.ts b/src/app/services/api.ts index cc2d71d5..c9f677f6 100644 --- a/src/app/services/api.ts +++ b/src/app/services/api.ts @@ -263,7 +263,7 @@ interface GraphQLIssueSearchResponse { pageInfo: { hasNextPage: boolean; endCursor: string | null }; nodes: (GraphQLIssueNode | null)[]; }; - rateLimit?: { remaining: number; resetAt: string }; + rateLimit?: { limit: number; remaining: number; resetAt: string }; } interface GraphQLPRNode { @@ -310,7 +310,7 @@ interface GraphQLPRSearchResponse { pageInfo: { hasNextPage: boolean; endCursor: string | null }; nodes: (GraphQLPRNode | null)[]; }; - rateLimit?: { remaining: number; resetAt: string }; + rateLimit?: { limit: number; remaining: number; resetAt: string }; } interface ForkCandidate { @@ -325,8 +325,8 @@ interface ForkRepoResult { } interface ForkQueryResponse { - rateLimit?: { remaining: number; resetAt: string }; - [key: string]: ForkRepoResult | { remaining: number; resetAt: string } | undefined | null; + rateLimit?: { limit: number; remaining: number; resetAt: string }; + [key: string]: ForkRepoResult | { limit: number; remaining: number; resetAt: string } | undefined | null; } // ── GraphQL search query constants ─────────────────────────────────────────── @@ -353,7 +353,7 @@ const ISSUES_SEARCH_QUERY = ` } } } - rateLimit { remaining resetAt } + rateLimit { limit remaining resetAt } } `; @@ -406,7 +406,7 @@ const PR_SEARCH_QUERY = ` } } } - rateLimit { remaining resetAt } + rateLimit { limit remaining resetAt } } `; @@ -479,7 +479,7 @@ const LIGHT_COMBINED_SEARCH_QUERY = ` } } } - rateLimit { remaining resetAt } + rateLimit { limit remaining resetAt } } ${LIGHT_PR_FRAGMENT} `; @@ -496,7 +496,7 @@ const LIGHT_PR_SEARCH_QUERY = ` } } } - rateLimit { remaining resetAt } + rateLimit { limit remaining resetAt } } ${LIGHT_PR_FRAGMENT} `; @@ -507,7 +507,7 @@ interface LightPRSearchResponse { pageInfo: { hasNextPage: boolean; endCursor: string | null }; nodes: (GraphQLLightPRNode | null)[]; }; - rateLimit?: { remaining: number; resetAt: string }; + rateLimit?: { limit: number; remaining: number; resetAt: string }; } /** Phase 2 backfill query: enriches PRs with heavy fields using node IDs. */ @@ -541,7 +541,7 @@ const HEAVY_PR_BACKFILL_QUERY = ` } } } - rateLimit { remaining resetAt } + rateLimit { limit remaining resetAt } } `; @@ -563,7 +563,7 @@ const HOT_PR_STATUS_QUERY = ` } } } - rateLimit { remaining resetAt } + rateLimit { limit remaining resetAt } } `; @@ -577,7 +577,7 @@ interface HotPRStatusNode { interface HotPRStatusResponse { nodes: (HotPRStatusNode | null)[]; - rateLimit?: { remaining: number; resetAt: string }; + rateLimit?: { limit: number; remaining: number; resetAt: string }; } interface GraphQLLightPRNode { @@ -639,12 +639,12 @@ interface LightCombinedSearchResponse { pageInfo: { hasNextPage: boolean; endCursor: string | null }; nodes: (GraphQLLightPRNode | null)[]; }; - rateLimit?: { remaining: number; resetAt: string }; + rateLimit?: { limit: number; remaining: number; resetAt: string }; } interface HeavyBackfillResponse { nodes: (GraphQLHeavyPRNode | null)[]; - rateLimit?: { remaining: number; resetAt: string }; + rateLimit?: { limit: number; remaining: number; resetAt: string }; } // Max node IDs per nodes() query (GitHub limit) @@ -663,7 +663,7 @@ interface SearchPageResult { * caller-provided `processNode` callback. Handles partial errors, cap enforcement, * and rate limit tracking. Returns the count of items added by processNode. */ -async function paginateGraphQLSearch; rateLimit?: { remaining: number; resetAt: string } }, TNode>( +async function paginateGraphQLSearch; rateLimit?: { limit: number; remaining: number; resetAt: string } }, TNode>( octokit: GitHubOctokit, query: string, queryString: string, @@ -815,11 +815,11 @@ async function runForkPRFallback( ); } - const forkQuery = `query(${varDefs.join(", ")}) {\n${fragments.join("\n")}\nrateLimit { remaining resetAt }\n}`; + const forkQuery = `query(${varDefs.join(", ")}) {\n${fragments.join("\n")}\nrateLimit { limit remaining resetAt }\n}`; try { const forkResponse = await octokit.graphql(forkQuery, variables); - if (forkResponse.rateLimit) updateGraphqlRateLimit(forkResponse.rateLimit as { remaining: number; resetAt: string }); + if (forkResponse.rateLimit) updateGraphqlRateLimit(forkResponse.rateLimit as { limit: number; remaining: number; resetAt: string }); for (let i = 0; i < forkChunk.length; i++) { const data = forkResponse[`fork${i}`] as ForkRepoResult | null | undefined; @@ -1475,11 +1475,11 @@ async function graphqlSearchPRs( ); } - const forkQuery = `query(${varDefs.join(", ")}) {\n${fragments.join("\n")}\nrateLimit { remaining resetAt }\n}`; + const forkQuery = `query(${varDefs.join(", ")}) {\n${fragments.join("\n")}\nrateLimit { limit remaining resetAt }\n}`; try { const forkResponse = await octokit.graphql(forkQuery, variables); - if (forkResponse.rateLimit) updateGraphqlRateLimit(forkResponse.rateLimit as { remaining: number; resetAt: string }); + if (forkResponse.rateLimit) updateGraphqlRateLimit(forkResponse.rateLimit as { limit: number; remaining: number; resetAt: string }); for (let i = 0; i < forkChunk.length; i++) { const data = forkResponse[`fork${i}`] as ForkRepoResult | null | undefined; diff --git a/src/app/services/github.ts b/src/app/services/github.ts index 45c7f626..af23f1ef 100644 --- a/src/app/services/github.ts +++ b/src/app/services/github.ts @@ -16,6 +16,7 @@ const GitHubOctokit = Octokit.plugin(throttling, retry, paginateRest); type GitHubOctokitInstance = InstanceType; interface RateLimitInfo { + limit: number; remaining: number; resetAt: Date; } @@ -33,8 +34,13 @@ export function getGraphqlRateLimit(): RateLimitInfo | null { return _graphqlRateLimit(); } -export function updateGraphqlRateLimit(rateLimit: { remaining: number; resetAt: string }): void { +function safePositiveInt(raw: number | undefined, fallback: number): number { + return raw != null && Number.isFinite(raw) && raw > 0 ? raw : fallback; +} + +export function updateGraphqlRateLimit(rateLimit: { limit: number; remaining: number; resetAt: string }): void { _setGraphqlRateLimit({ + limit: safePositiveInt(rateLimit.limit, _graphqlRateLimit()?.limit ?? 5000), remaining: rateLimit.remaining, resetAt: new Date(rateLimit.resetAt), // ISO 8601 string → Date }); @@ -43,8 +49,11 @@ export function updateGraphqlRateLimit(rateLimit: { remaining: number; resetAt: export function updateRateLimitFromHeaders(headers: Record): void { const remaining = headers["x-ratelimit-remaining"]; const reset = headers["x-ratelimit-reset"]; + const limit = headers["x-ratelimit-limit"]; if (remaining !== undefined && reset !== undefined) { + const parsedLimit = limit !== undefined ? parseInt(limit, 10) : NaN; _setCoreRateLimit({ + limit: safePositiveInt(parsedLimit, 5000), remaining: parseInt(remaining, 10), resetAt: new Date(parseInt(reset, 10) * 1000), }); diff --git a/tests/integration/data-pipeline.test.ts b/tests/integration/data-pipeline.test.ts index 80b0f1fa..e18a3809 100644 --- a/tests/integration/data-pipeline.test.ts +++ b/tests/integration/data-pipeline.test.ts @@ -64,7 +64,7 @@ function makeGraphqlSearchResponse(nodes = [graphqlIssueNode]) { pageInfo: { hasNextPage: false, endCursor: null }, nodes, }, - rateLimit: { remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, }; } diff --git a/tests/services/api-optimization.test.ts b/tests/services/api-optimization.test.ts index 29fee11a..1ba11fcd 100644 --- a/tests/services/api-optimization.test.ts +++ b/tests/services/api-optimization.test.ts @@ -108,7 +108,7 @@ function makeHeavyPRNode(databaseId: number, _nodeId?: string) { }; } -const rateLimit = { remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }; +const rateLimit = { limit: 5000, remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }; function makeSearchResponse(nodes: T[], hasNextPage = false) { return { diff --git a/tests/services/api.test.ts b/tests/services/api.test.ts index 1213d7b8..13affff2 100644 --- a/tests/services/api.test.ts +++ b/tests/services/api.test.ts @@ -193,7 +193,7 @@ function makeGraphqlIssueResponse(nodes = [graphqlIssueNode], hasNextPage = fals pageInfo: { hasNextPage, endCursor: hasNextPage ? "cursor1" : null }, nodes, }, - rateLimit: { remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, }; } @@ -371,7 +371,7 @@ describe("fetchIssues", () => { pageInfo: { hasNextPage: false, endCursor: null }, nodes: [graphqlIssueNode, null, { ...graphqlIssueNode, databaseId: 999 }], }, - rateLimit: { remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, })); const result = await fetchIssues( @@ -427,7 +427,7 @@ describe("fetchIssues", () => { pageInfo: { hasNextPage: true, endCursor: "cursor-partial" }, nodes: [{ ...graphqlIssueNode, databaseId: 42 }, null], }, - rateLimit: { remaining: 4990, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 4990, resetAt: new Date(Date.now() + 3600000).toISOString() }, }, }); const octokit = makeIssueOctokit(async () => { @@ -563,7 +563,7 @@ function makeGraphqlPRResponse(nodes = [graphqlPRNode], hasNextPage = false, iss pageInfo: { hasNextPage, endCursor: hasNextPage ? "cursor1" : null }, nodes, }, - rateLimit: { remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, }; } @@ -714,7 +714,7 @@ describe("fetchPullRequests", () => { // Fork fallback query return { fork0: { object: { statusCheckRollup: { state: "SUCCESS" } } }, - rateLimit: { remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, }; }); @@ -788,7 +788,7 @@ describe("fetchPullRequests", () => { pageInfo: { hasNextPage: true, endCursor: null }, // degenerate response nodes: [{ ...graphqlPRNode, databaseId: callCount }], }, - rateLimit: { remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, }; }); @@ -874,7 +874,7 @@ describe("fetchPullRequests", () => { data: { fork0: { object: { statusCheckRollup: { state: "SUCCESS" } } }, // fork1 is missing — that fork repo was deleted/inaccessible - rateLimit: { remaining: 4990, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 4990, resetAt: new Date(Date.now() + 3600000).toISOString() }, }, }); }); @@ -951,7 +951,7 @@ describe("fetchPullRequests", () => { pageInfo: { hasNextPage: true, endCursor: "cursor-partial" }, nodes: [{ ...graphqlPRNode, databaseId: 77 }], }, - rateLimit: { remaining: 4990, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 4990, resetAt: new Date(Date.now() + 3600000).toISOString() }, }, }); const octokit = makePROctokit(async (_query, variables) => { @@ -1038,7 +1038,7 @@ describe("fetchPullRequests", () => { } // Fork fallback queries const response: Record = { - rateLimit: { remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() }, }; const indices = Object.keys(variables as Record) .filter((k) => k.startsWith("owner")) diff --git a/tests/services/github.test.ts b/tests/services/github.test.ts index 412eabb3..a011e643 100644 --- a/tests/services/github.test.ts +++ b/tests/services/github.test.ts @@ -305,18 +305,27 @@ describe("getGraphqlRateLimit", () => { it("converts ISO 8601 resetAt string to Date", () => { const iso = "2024-06-01T12:00:00Z"; - updateGraphqlRateLimit({ remaining: 4500, resetAt: iso }); + updateGraphqlRateLimit({ limit: 5000, remaining: 4500, resetAt: iso }); const rl = getGraphqlRateLimit(); expect(rl).not.toBeNull(); + expect(rl!.limit).toBe(5000); expect(rl!.remaining).toBe(4500); expect(rl!.resetAt).toBeInstanceOf(Date); expect(rl!.resetAt.getTime()).toBe(new Date(iso).getTime()); }); + it("stores limit from Enterprise Cloud (10000)", () => { + updateGraphqlRateLimit({ limit: 10000, remaining: 9500, resetAt: "2024-06-01T12:00:00Z" }); + const rl = getGraphqlRateLimit(); + expect(rl!.limit).toBe(10000); + expect(rl!.remaining).toBe(9500); + }); + it("overwrites previous value on subsequent updates", () => { - updateGraphqlRateLimit({ remaining: 5000, resetAt: "2024-06-01T12:00:00Z" }); - updateGraphqlRateLimit({ remaining: 3000, resetAt: "2024-06-01T13:00:00Z" }); + updateGraphqlRateLimit({ limit: 5000, remaining: 5000, resetAt: "2024-06-01T12:00:00Z" }); + updateGraphqlRateLimit({ limit: 5000, remaining: 3000, resetAt: "2024-06-01T13:00:00Z" }); const rl = getGraphqlRateLimit(); + expect(rl!.limit).toBe(5000); expect(rl!.remaining).toBe(3000); }); }); diff --git a/tests/services/hot-poll.test.ts b/tests/services/hot-poll.test.ts index e892e30b..ddeb9f05 100644 --- a/tests/services/hot-poll.test.ts +++ b/tests/services/hot-poll.test.ts @@ -74,7 +74,7 @@ function makeOctokit( ) { return { request: vi.fn(requestImpl ?? (() => Promise.resolve({ data: {}, headers: {} }))), - graphql: vi.fn(graphqlImpl ?? (() => Promise.resolve({ nodes: [], rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" } }))), + graphql: vi.fn(graphqlImpl ?? (() => Promise.resolve({ nodes: [], rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" } }))), hook: { before: vi.fn() }, }; } @@ -106,7 +106,7 @@ describe("fetchHotPRStatus", () => { reviewDecision: "APPROVED", commits: { nodes: [{ commit: { statusCheckRollup: { state: "SUCCESS" } } }] }, }], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, })); const { results } = await fetchHotPRStatus(octokit as never, ["PR_node1"]); @@ -127,7 +127,7 @@ describe("fetchHotPRStatus", () => { reviewDecision: null, commits: { nodes: [{ commit: { statusCheckRollup: { state: "SUCCESS" } } }] }, }], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, })); const { results } = await fetchHotPRStatus(octokit as never, ["PR_node2"]); @@ -143,7 +143,7 @@ describe("fetchHotPRStatus", () => { reviewDecision: null, commits: { nodes: [{ commit: { statusCheckRollup: { state: "PENDING" } } }] }, }], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, })); const { results } = await fetchHotPRStatus(octokit as never, ["PR_node3"]); @@ -238,7 +238,7 @@ describe("rebuildHotSets", () => { it("populates hot PRs for pending/null checkStatus with nodeId", async () => { const octokit = makeOctokit(undefined, () => Promise.resolve({ nodes: [], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, })); mockGetClient.mockReturnValue(octokit); @@ -311,7 +311,7 @@ describe("rebuildHotSets", () => { data: { id: 1, status: "in_progress", conclusion: null, updated_at: "2026-01-01T00:00:00Z", completed_at: null }, headers: {}, }), - () => Promise.resolve({ nodes: [], rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" } }), + () => Promise.resolve({ nodes: [], rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" } }), ); mockGetClient.mockReturnValue(octokit); @@ -370,7 +370,7 @@ describe("fetchHotData", () => { reviewDecision: "APPROVED", commits: { nodes: [{ commit: { statusCheckRollup: { state: "SUCCESS" } } }] }, }], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, })); const octokit = makeOctokit(undefined, graphqlFn); mockGetClient.mockReturnValue(octokit); @@ -684,7 +684,7 @@ describe("fetchHotPRStatus null/missing nodes", () => { commits: { nodes: [{ commit: { statusCheckRollup: { state: "SUCCESS" } } }] }, }, ], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, })); const { results } = await fetchHotPRStatus(octokit as never, ["PR_null", "PR_valid"]); @@ -698,7 +698,7 @@ describe("fetchHotPRStatus null/missing nodes", () => { { databaseId: null, state: "OPEN", mergeStateStatus: "CLEAN", reviewDecision: null, commits: { nodes: [] } }, { databaseId: 77, state: "OPEN", mergeStateStatus: "CLEAN", reviewDecision: null, commits: { nodes: [{ commit: { statusCheckRollup: { state: "PENDING" } } }] } }, ], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, })); const { results } = await fetchHotPRStatus(octokit as never, ["PR_nulldb", "PR_ok"]); @@ -717,7 +717,7 @@ describe("fetchHotPRStatus edge cases", () => { reviewDecision: null, commits: { nodes: [{ commit: { statusCheckRollup: { state: "SUCCESS" } } }] }, }], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, })); const { results } = await fetchHotPRStatus(octokit as never, ["PR_behind"]); @@ -737,7 +737,7 @@ describe("fetchHotPRStatus edge cases", () => { reviewDecision: null, commits: { nodes: [{ commit: { statusCheckRollup: { state: "PENDING" } } }] }, }], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, }); } return Promise.reject(new Error("rate limited")); @@ -768,7 +768,7 @@ describe("rebuildHotSets caps", () => { const octokit = makeOctokit(undefined, () => Promise.resolve({ nodes: [], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, })); mockGetClient.mockReturnValue(octokit); @@ -820,7 +820,7 @@ describe("fetchHotData eviction edge cases", () => { { databaseId: 1, state: "OPEN", mergeStateStatus: "CLEAN", reviewDecision: null, commits: { nodes: [{ commit: { statusCheckRollup: { state: "SUCCESS" } } }] } }, { databaseId: 2, state: "OPEN", mergeStateStatus: "CLEAN", reviewDecision: null, commits: { nodes: [{ commit: { statusCheckRollup: { state: "PENDING" } } }] } }, ], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, }); } // Second fetch: only PR 2 should be queried @@ -828,7 +828,7 @@ describe("fetchHotData eviction edge cases", () => { nodes: [ { databaseId: 2, state: "OPEN", mergeStateStatus: "CLEAN", reviewDecision: null, commits: { nodes: [{ commit: { statusCheckRollup: { state: "SUCCESS" } } }] } }, ], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, }); }); const octokit = makeOctokit(undefined, graphqlFn); @@ -864,7 +864,7 @@ describe("fetchHotData eviction edge cases", () => { reviewDecision: "APPROVED", commits: { nodes: [{ commit: { statusCheckRollup: { state: "PENDING" } } }] }, }], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, })); const octokit = makeOctokit(undefined, graphqlFn); mockGetClient.mockReturnValue(octokit); @@ -893,7 +893,7 @@ describe("fetchHotData eviction edge cases", () => { reviewDecision: null, commits: { nodes: [{ commit: { statusCheckRollup: null } }] }, }], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, })); const octokit = makeOctokit(undefined, graphqlFn); mockGetClient.mockReturnValue(octokit); @@ -945,7 +945,7 @@ describe("fetchHotData hadErrors", () => { }), () => Promise.resolve({ nodes: [{ databaseId: 1, state: "OPEN", mergeStateStatus: "CLEAN", reviewDecision: null, commits: { nodes: [{ commit: { statusCheckRollup: { state: "PENDING" } } }] } }], - rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" }, }), ); mockGetClient.mockReturnValue(octokit); @@ -977,7 +977,7 @@ describe("fetchHotData hadErrors", () => { it("returns hadErrors=true when a run fetch fails", async () => { const octokit = makeOctokit( () => Promise.reject(new Error("network error")), - () => Promise.resolve({ nodes: [], rateLimit: { remaining: 4999, resetAt: "2026-01-01T00:00:00Z" } }), + () => Promise.resolve({ nodes: [], rateLimit: { limit: 5000, remaining: 4999, resetAt: "2026-01-01T00:00:00Z" } }), ); mockGetClient.mockReturnValue(octokit); @@ -997,10 +997,10 @@ describe("fetchHotPRStatus updateGraphqlRateLimit", () => { const { updateGraphqlRateLimit } = await import("../../src/app/services/github"); const octokit = makeOctokit(undefined, () => Promise.resolve({ nodes: [{ databaseId: 1, state: "OPEN", mergeStateStatus: "CLEAN", reviewDecision: null, commits: { nodes: [{ commit: { statusCheckRollup: { state: "SUCCESS" } } }] } }], - rateLimit: { remaining: 4200, resetAt: "2026-01-01T01:00:00Z" }, + rateLimit: { limit: 5000, remaining: 4200, resetAt: "2026-01-01T01:00:00Z" }, })); await fetchHotPRStatus(octokit as never, ["PR_rl"]); - expect(updateGraphqlRateLimit).toHaveBeenCalledWith({ remaining: 4200, resetAt: "2026-01-01T01:00:00Z" }); + expect(updateGraphqlRateLimit).toHaveBeenCalledWith({ limit: 5000, remaining: 4200, resetAt: "2026-01-01T01:00:00Z" }); }); }); From ceab1789ed5910cb1726246231d7fa7ee02b70e0 Mon Sep 17 00:00:00 2001 From: testvalue Date: Mon, 30 Mar 2026 11:27:25 -0400 Subject: [PATCH 4/4] fix(config): addresses code review findings - inlines one-use keysProvided variable in updateConfig - adds updateRateLimitFromHeaders unit tests (valid, missing, malformed header) - adds safePositiveInt edge case tests (zero, negative limit fallback) - documents resetConfig() safety outside reactive root --- src/app/stores/config.ts | 5 ++-- tests/services/github.test.ts | 54 ++++++++++++++++++++++++++++++++++- tests/stores/config.test.ts | 1 + 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/src/app/stores/config.ts b/src/app/stores/config.ts index 7cdd6a04..71ba5f96 100644 --- a/src/app/stores/config.ts +++ b/src/app/stores/config.ts @@ -76,8 +76,9 @@ export function updateConfig(partial: Partial): void { // Only merge keys the caller actually provided: Zod .partial().safeParse() // still applies per-field .default() values for absent keys, inflating // validated.data with defaults that would overwrite live state. - const keysProvided = Object.keys(partial) as (keyof Config)[]; - const filtered = Object.fromEntries(keysProvided.map((k) => [k, validated.data[k]])); + const filtered = Object.fromEntries( + (Object.keys(partial) as (keyof Config)[]).map((k) => [k, validated.data[k]]) + ); setConfig( produce((draft) => { Object.assign(draft, filtered); diff --git a/tests/services/github.test.ts b/tests/services/github.test.ts index a011e643..0a067942 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 } from "vitest"; import { createRoot } from "solid-js"; -import { createGitHubClient, cachedRequest, getClient, initClientWatcher, getGraphqlRateLimit, updateGraphqlRateLimit } from "../../src/app/services/github"; +import { createGitHubClient, cachedRequest, getClient, initClientWatcher, getGraphqlRateLimit, updateGraphqlRateLimit, updateRateLimitFromHeaders, getCoreRateLimit } from "../../src/app/services/github"; import { clearCache } from "../../src/app/stores/cache"; // ── createGitHubClient ─────────────────────────────────────────────────────── @@ -328,4 +328,56 @@ describe("getGraphqlRateLimit", () => { expect(rl!.limit).toBe(5000); expect(rl!.remaining).toBe(3000); }); + + it("falls back to previous limit when zero is provided", () => { + updateGraphqlRateLimit({ limit: 5000, remaining: 100, resetAt: "2024-06-01T12:00:00Z" }); + updateGraphqlRateLimit({ limit: 0, remaining: 50, resetAt: "2024-06-01T13:00:00Z" }); + const rl = getGraphqlRateLimit(); + expect(rl!.limit).toBe(5000); + expect(rl!.remaining).toBe(50); + }); + + it("falls back to previous limit when negative is provided", () => { + updateGraphqlRateLimit({ limit: 10000, remaining: 100, resetAt: "2024-06-01T12:00:00Z" }); + updateGraphqlRateLimit({ limit: -1, remaining: 50, resetAt: "2024-06-01T13:00:00Z" }); + const rl = getGraphqlRateLimit(); + expect(rl!.limit).toBe(10000); + }); +}); + +// ── updateRateLimitFromHeaders ─────────────────────────────────────────────── + +describe("updateRateLimitFromHeaders", () => { + it("parses limit from x-ratelimit-limit header", () => { + updateRateLimitFromHeaders({ + "x-ratelimit-remaining": "4500", + "x-ratelimit-reset": String(Math.floor(Date.now() / 1000) + 3600), + "x-ratelimit-limit": "10000", + }); + const rl = getCoreRateLimit(); + expect(rl).not.toBeNull(); + expect(rl!.limit).toBe(10000); + expect(rl!.remaining).toBe(4500); + }); + + it("falls back to 5000 when x-ratelimit-limit header is absent", () => { + updateRateLimitFromHeaders({ + "x-ratelimit-remaining": "4999", + "x-ratelimit-reset": String(Math.floor(Date.now() / 1000) + 3600), + }); + const rl = getCoreRateLimit(); + expect(rl).not.toBeNull(); + expect(rl!.limit).toBe(5000); + }); + + it("falls back to 5000 when x-ratelimit-limit header is malformed", () => { + updateRateLimitFromHeaders({ + "x-ratelimit-remaining": "4999", + "x-ratelimit-reset": String(Math.floor(Date.now() / 1000) + 3600), + "x-ratelimit-limit": "abc", + }); + const rl = getCoreRateLimit(); + expect(rl).not.toBeNull(); + expect(rl!.limit).toBe(5000); + }); }); diff --git a/tests/stores/config.test.ts b/tests/stores/config.test.ts index 232767bf..5993d9ee 100644 --- a/tests/stores/config.test.ts +++ b/tests/stores/config.test.ts @@ -213,6 +213,7 @@ describe("store merge behavior (produce + Object.assign)", () => { }); describe("updateConfig (real export)", () => { + // resetConfig() only calls setConfig(defaults) — no reactive effects, safe outside a root beforeEach(() => { resetConfig(); });