Skip to content

Commit da0b121

Browse files
committed
fix(api): adds missing tests and fork partial-error recovery
1 parent 4437f50 commit da0b121

File tree

3 files changed

+166
-1
lines changed

3 files changed

+166
-1
lines changed

src/app/services/api.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -704,7 +704,22 @@ async function graphqlSearchPRs(
704704
if (pr) pr.checkStatus = mapCheckStatus(state);
705705
}
706706
} catch (err) {
707-
console.warn("[api] Fork PR statusCheckRollup fallback failed:", err);
707+
// Extract partial data from GraphqlResponseError — some fork aliases may have resolved
708+
const partialData = (err && typeof err === "object" && "data" in err && err.data && typeof err.data === "object")
709+
? err.data as Record<string, ForkRepoResult | null | undefined>
710+
: null;
711+
712+
if (partialData) {
713+
for (let i = 0; i < forkChunk.length; i++) {
714+
const data = partialData[`fork${i}`];
715+
if (!data) continue;
716+
const state = data.object?.statusCheckRollup?.state ?? null;
717+
const pr = prMap.get(forkChunk[i].databaseId);
718+
if (pr) pr.checkStatus = mapCheckStatus(state);
719+
}
720+
}
721+
722+
console.warn("[api] Fork PR statusCheckRollup fallback partially failed:", err);
708723
pushNotification("graphql", "Fork PR check status unavailable — CI status may be missing for some PRs", "warning");
709724
}
710725
}));

tests/services/api.test.ts

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,56 @@ describe("fetchIssues", () => {
469469
expect(result.errors[0].retryable).toBe(false);
470470
});
471471

472+
it("rejects invalid userLogin with error instead of injecting into query", async () => {
473+
const octokit = makeIssueOctokit();
474+
const result = await fetchIssues(
475+
octokit as unknown as ReturnType<typeof import("../../src/app/services/github").getClient>,
476+
[testRepo],
477+
"bad user" // contains space — fails VALID_LOGIN
478+
);
479+
480+
expect(result.issues).toEqual([]);
481+
expect(result.errors.length).toBe(1);
482+
expect(result.errors[0].message).toContain("Invalid userLogin");
483+
expect(octokit.graphql).not.toHaveBeenCalled();
484+
});
485+
486+
it("truncates to exactly 1000 when parallel chunks overshoot", async () => {
487+
vi.mocked(pushNotification).mockClear();
488+
489+
// 35 repos → 2 chunks. Each chunk returns 600 items (total 1200, well over cap).
490+
const repos: RepoRef[] = Array.from({ length: 35 }, (_, i) => ({
491+
owner: "org",
492+
name: `repo-${i}`,
493+
fullName: `org/repo-${i}`,
494+
}));
495+
496+
let callCount = 0;
497+
const octokit = makeIssueOctokit(async () => {
498+
callCount++;
499+
const batchStart = (callCount - 1) * 600;
500+
const nodes = Array.from({ length: 600 }, (_, i) => ({
501+
...graphqlIssueNode,
502+
databaseId: batchStart + i + 1,
503+
}));
504+
return makeGraphqlIssueResponse(nodes, false, 600);
505+
});
506+
507+
const result = await fetchIssues(
508+
octokit as unknown as ReturnType<typeof import("../../src/app/services/github").getClient>,
509+
repos,
510+
"octocat"
511+
);
512+
513+
// splice(1000) ensures exactly 1000 even with parallel overshoot
514+
expect(result.issues.length).toBe(1000);
515+
expect(pushNotification).toHaveBeenCalledWith(
516+
"search/issues",
517+
expect.stringContaining("capped at 1,000"),
518+
"warning"
519+
);
520+
});
521+
472522
it("throws when octokit is null", async () => {
473523
await expect(fetchIssues(null, [testRepo], "octocat")).rejects.toThrow(
474524
"No GitHub client available"
@@ -794,6 +844,61 @@ describe("fetchPullRequests", () => {
794844
);
795845
});
796846

847+
it("recovers partial data from fork fallback GraphqlResponseError", async () => {
848+
vi.mocked(pushNotification).mockClear();
849+
850+
// Two fork PRs: one resolves in partial data, one doesn't
851+
const forkNodes = [
852+
{
853+
...graphqlPRNode, databaseId: 901,
854+
headRepository: { owner: { login: "fork-a" }, nameWithOwner: "fork-a/repo" },
855+
repository: { nameWithOwner: "octocat/Hello-World" },
856+
commits: { nodes: [{ commit: { statusCheckRollup: null } }] },
857+
},
858+
{
859+
...graphqlPRNode, databaseId: 902,
860+
headRepository: { owner: { login: "fork-b" }, nameWithOwner: "fork-b/repo" },
861+
repository: { nameWithOwner: "octocat/Hello-World" },
862+
commits: { nodes: [{ commit: { statusCheckRollup: null } }] },
863+
},
864+
] as unknown as (typeof graphqlPRNode)[];
865+
866+
let graphqlCallCount = 0;
867+
const octokit = makePROctokit(async () => {
868+
graphqlCallCount++;
869+
if (graphqlCallCount <= 2) {
870+
return makeGraphqlPRResponse(forkNodes);
871+
}
872+
// Fork fallback: GraphqlResponseError with partial data — fork0 resolves, fork1 doesn't
873+
throw Object.assign(new Error("Partial fork resolution failure"), {
874+
data: {
875+
fork0: { object: { statusCheckRollup: { state: "SUCCESS" } } },
876+
// fork1 is missing — that fork repo was deleted/inaccessible
877+
rateLimit: { remaining: 4990, resetAt: new Date(Date.now() + 3600000).toISOString() },
878+
},
879+
});
880+
});
881+
882+
const { pullRequests } = await fetchPullRequests(
883+
octokit as unknown as ReturnType<typeof import("../../src/app/services/github").getClient>,
884+
[testRepo],
885+
"octocat"
886+
);
887+
888+
const pr901 = pullRequests.find((pr) => pr.id === 901);
889+
const pr902 = pullRequests.find((pr) => pr.id === 902);
890+
// fork0 resolved from partial data
891+
expect(pr901?.checkStatus).toBe("success");
892+
// fork1 not in partial data — stays null
893+
expect(pr902?.checkStatus).toBeNull();
894+
// Notification still fires for the partial failure
895+
expect(pushNotification).toHaveBeenCalledWith(
896+
"graphql",
897+
expect.stringContaining("Fork PR check status unavailable"),
898+
"warning"
899+
);
900+
});
901+
797902
it("returns partial results and an error when second page throws mid-pagination", async () => {
798903
vi.mocked(pushNotification).mockClear();
799904

tests/services/poll-fetchAllData.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -580,3 +580,48 @@ describe("fetchAllData — notification gate 403 auto-disable", () => {
580580
expect(fetchWorkflowRuns).toHaveBeenCalled();
581581
});
582582
});
583+
584+
// ── qa-4: Concurrency verification ────────────────────────────────────────────
585+
586+
describe("fetchAllData — parallel execution", () => {
587+
it("initiates all three fetches before any resolves", async () => {
588+
vi.resetModules();
589+
590+
const { getClient } = await import("../../src/app/services/github");
591+
const { fetchIssues, fetchPullRequests, fetchWorkflowRuns } = await import("../../src/app/services/api");
592+
const mockOctokit = makeMockOctokit();
593+
vi.mocked(getClient).mockReturnValue(mockOctokit as unknown as ReturnType<typeof getClient>);
594+
595+
const callOrder: string[] = [];
596+
const resolvers: Array<(v: unknown) => void> = [];
597+
598+
// Each mock records when it's called but doesn't resolve until manually triggered
599+
vi.mocked(fetchIssues).mockImplementation(() => {
600+
callOrder.push("issues-start");
601+
return new Promise((resolve) => { resolvers.push(() => resolve(emptyIssueResult)); });
602+
});
603+
vi.mocked(fetchPullRequests).mockImplementation(() => {
604+
callOrder.push("prs-start");
605+
return new Promise((resolve) => { resolvers.push(() => resolve(emptyPrResult)); });
606+
});
607+
vi.mocked(fetchWorkflowRuns).mockImplementation(() => {
608+
callOrder.push("runs-start");
609+
return new Promise((resolve) => { resolvers.push(() => resolve(emptyRunResult)); });
610+
});
611+
612+
const { fetchAllData } = await import("../../src/app/services/poll");
613+
614+
const promise = fetchAllData();
615+
616+
// Yield to allow Promise.allSettled to initiate all three
617+
await new Promise((r) => setTimeout(r, 0));
618+
619+
// All three should have been called BEFORE any resolved
620+
expect(callOrder).toEqual(["issues-start", "prs-start", "runs-start"]);
621+
expect(resolvers.length).toBe(3);
622+
623+
// Now resolve all
624+
for (const resolve of resolvers) resolve(undefined);
625+
await promise;
626+
});
627+
});

0 commit comments

Comments
 (0)