Skip to content

Commit ceab178

Browse files
committed
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
1 parent f8cdb7c commit ceab178

File tree

3 files changed

+57
-3
lines changed

3 files changed

+57
-3
lines changed

src/app/stores/config.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,9 @@ export function updateConfig(partial: Partial<Config>): void {
7676
// Only merge keys the caller actually provided: Zod .partial().safeParse()
7777
// still applies per-field .default() values for absent keys, inflating
7878
// validated.data with defaults that would overwrite live state.
79-
const keysProvided = Object.keys(partial) as (keyof Config)[];
80-
const filtered = Object.fromEntries(keysProvided.map((k) => [k, validated.data[k]]));
79+
const filtered = Object.fromEntries(
80+
(Object.keys(partial) as (keyof Config)[]).map((k) => [k, validated.data[k]])
81+
);
8182
setConfig(
8283
produce((draft) => {
8384
Object.assign(draft, filtered);

tests/services/github.test.ts

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import "fake-indexeddb/auto";
22
import { describe, it, expect, vi, beforeEach } from "vitest";
33
import { createRoot } from "solid-js";
4-
import { createGitHubClient, cachedRequest, getClient, initClientWatcher, getGraphqlRateLimit, updateGraphqlRateLimit } from "../../src/app/services/github";
4+
import { createGitHubClient, cachedRequest, getClient, initClientWatcher, getGraphqlRateLimit, updateGraphqlRateLimit, updateRateLimitFromHeaders, getCoreRateLimit } from "../../src/app/services/github";
55
import { clearCache } from "../../src/app/stores/cache";
66

77
// ── createGitHubClient ───────────────────────────────────────────────────────
@@ -328,4 +328,56 @@ describe("getGraphqlRateLimit", () => {
328328
expect(rl!.limit).toBe(5000);
329329
expect(rl!.remaining).toBe(3000);
330330
});
331+
332+
it("falls back to previous limit when zero is provided", () => {
333+
updateGraphqlRateLimit({ limit: 5000, remaining: 100, resetAt: "2024-06-01T12:00:00Z" });
334+
updateGraphqlRateLimit({ limit: 0, remaining: 50, resetAt: "2024-06-01T13:00:00Z" });
335+
const rl = getGraphqlRateLimit();
336+
expect(rl!.limit).toBe(5000);
337+
expect(rl!.remaining).toBe(50);
338+
});
339+
340+
it("falls back to previous limit when negative is provided", () => {
341+
updateGraphqlRateLimit({ limit: 10000, remaining: 100, resetAt: "2024-06-01T12:00:00Z" });
342+
updateGraphqlRateLimit({ limit: -1, remaining: 50, resetAt: "2024-06-01T13:00:00Z" });
343+
const rl = getGraphqlRateLimit();
344+
expect(rl!.limit).toBe(10000);
345+
});
346+
});
347+
348+
// ── updateRateLimitFromHeaders ───────────────────────────────────────────────
349+
350+
describe("updateRateLimitFromHeaders", () => {
351+
it("parses limit from x-ratelimit-limit header", () => {
352+
updateRateLimitFromHeaders({
353+
"x-ratelimit-remaining": "4500",
354+
"x-ratelimit-reset": String(Math.floor(Date.now() / 1000) + 3600),
355+
"x-ratelimit-limit": "10000",
356+
});
357+
const rl = getCoreRateLimit();
358+
expect(rl).not.toBeNull();
359+
expect(rl!.limit).toBe(10000);
360+
expect(rl!.remaining).toBe(4500);
361+
});
362+
363+
it("falls back to 5000 when x-ratelimit-limit header is absent", () => {
364+
updateRateLimitFromHeaders({
365+
"x-ratelimit-remaining": "4999",
366+
"x-ratelimit-reset": String(Math.floor(Date.now() / 1000) + 3600),
367+
});
368+
const rl = getCoreRateLimit();
369+
expect(rl).not.toBeNull();
370+
expect(rl!.limit).toBe(5000);
371+
});
372+
373+
it("falls back to 5000 when x-ratelimit-limit header is malformed", () => {
374+
updateRateLimitFromHeaders({
375+
"x-ratelimit-remaining": "4999",
376+
"x-ratelimit-reset": String(Math.floor(Date.now() / 1000) + 3600),
377+
"x-ratelimit-limit": "abc",
378+
});
379+
const rl = getCoreRateLimit();
380+
expect(rl).not.toBeNull();
381+
expect(rl!.limit).toBe(5000);
382+
});
331383
});

tests/stores/config.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,7 @@ describe("store merge behavior (produce + Object.assign)", () => {
213213
});
214214

215215
describe("updateConfig (real export)", () => {
216+
// resetConfig() only calls setConfig(defaults) — no reactive effects, safe outside a root
216217
beforeEach(() => {
217218
resetConfig();
218219
});

0 commit comments

Comments
 (0)