From 1bea09960dc96ab0c655949b72eca5f418e489c9 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Fri, 8 May 2026 23:33:37 -0700 Subject: [PATCH 1/3] fix(host-service): replace bulky GraphQL PR query with targeted REST per-head lookups The GraphQL `PullRequestsForSidebar` query that fetched up to 100 PRs plus their `statusCheckRollup` in a single roundtrip times out (504) on large repos with heavy CI. Failures are cached for the full TTL, so the sidebar PR badge never resolves. Switch to per-upstream-branch REST queries (PR by head + reviews + checks/statuses), executed via `gh` with an Octokit fallback. Cache key is now (repo, head) so multiple workspaces tracking the same upstream branch still collapse to one call per TTL window. Fixes #4246 --- packages/host-service/src/app.ts | 1 + .../pull-requests/pull-requests.test.ts | 3 + .../runtime/pull-requests/pull-requests.ts | 220 +++++++++--- .../utils/github-query/github-query.test.ts | 217 ++++++++++++ .../utils/github-query/github-query.ts | 334 +++++++++++++++++- .../pull-requests/utils/github-query/index.ts | 15 +- .../pull-requests/utils/github-query/query.ts | 48 --- .../pull-requests/utils/github-query/types.ts | 34 +- .../pull-request-mappers.ts | 17 +- 9 files changed, 748 insertions(+), 141 deletions(-) create mode 100644 packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts delete mode 100644 packages/host-service/src/runtime/pull-requests/utils/github-query/query.ts diff --git a/packages/host-service/src/app.ts b/packages/host-service/src/app.ts index a2fd4f02730..65e1def808f 100644 --- a/packages/host-service/src/app.ts +++ b/packages/host-service/src/app.ts @@ -91,6 +91,7 @@ export function createApp(options: CreateAppOptions): CreateAppResult { gitWatcher.start(); const pullRequestRuntime = new PullRequestRuntimeManager({ db, + execGh, git, github, gitWatcher, diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts index 4280b20a990..8cd5304e8d5 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts @@ -142,6 +142,9 @@ function createFakeDb(state: FakeState) { function createManager(state: FakeState) { return new PullRequestRuntimeManager({ db: createFakeDb(state) as never, + execGh: async () => { + throw new Error("gh should not be used for direct PR linking"); + }, git: async () => { throw new Error("git should not be used when project metadata is set"); }, diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.ts index 522fec6f399..c1039fc77f4 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.ts @@ -5,9 +5,21 @@ import { and, eq, inArray } from "drizzle-orm"; import type { HostDb } from "../../db"; import { projects, pullRequests, workspaces } from "../../db/schema"; import type { GitWatcher } from "../../events/git-watcher"; +import type { ExecGh } from "../../trpc/router/workspace-creation/utils/exec-gh"; import type { GitFactory } from "../git"; -import { fetchRepositoryPullRequests } from "./utils/github-query"; -import type { GraphQLPullRequestNode } from "./utils/github-query/types"; +import { + fetchPullRequestByHead, + fetchPullRequestByHeadFromGh, + fetchPullRequestChecks, + fetchPullRequestChecksFromGh, + fetchPullRequestReviewDecision, + fetchPullRequestReviewDecisionFromGh, +} from "./utils/github-query"; +import type { + GitHubPullRequestHeadRef, + GitHubPullRequestNode, + GitHubPullRequestReviewDecision, +} from "./utils/github-query/types"; import { type ChecksStatus, coerceChecksStatus, @@ -35,9 +47,7 @@ const SAFETY_NET_INTERVAL_MS = 5 * 60_000; const PROJECT_REFRESH_INTERVAL_MS = 5 * 60_000; // Must exceed every polling interval that hits this cache (SAFETY_NET and // PROJECT_REFRESH). Otherwise the cache is always stale at poll time and -// each tick fires a fresh GraphQL call per repo. Multiple projects can -// target the same GitHub repo; this collapses them into one call per repo -// per TTL window. +// each tick fires fresh GitHub calls for the same upstream branch. const REPO_PULL_REQUEST_CACHE_TTL_MS = 60_000; const UNBORN_HEAD_ERROR_PATTERNS = [ "ambiguous argument 'head'", @@ -194,6 +204,7 @@ export interface PullRequestWorkspaceSnapshot { export interface PullRequestRuntimeManagerOptions { db: HostDb; + execGh: ExecGh; git: GitFactory; github: () => Promise; gitWatcher: GitWatcher; @@ -248,6 +259,7 @@ function deriveCheckoutPullRequestUpstream( export class PullRequestRuntimeManager { private readonly db: HostDb; + private readonly execGh: ExecGh; private readonly git: GitFactory; private readonly github: () => Promise; private readonly gitWatcher: GitWatcher; @@ -259,13 +271,14 @@ export class PullRequestRuntimeManager { string, { running: Promise; rerunPending: boolean } >(); - private readonly repoPullRequestCache = new Map< + private readonly pullRequestHeadCache = new Map< string, - { promise: Promise; fetchedAt: number } + { promise: Promise; fetchedAt: number } >(); constructor(options: PullRequestRuntimeManagerOptions) { this.db = options.db; + this.execGh = options.execGh; this.git = options.git; this.github = options.github; this.gitWatcher = options.gitWatcher; @@ -610,20 +623,25 @@ export class PullRequestRuntimeManager { .all(); if (projectWorkspaces.length === 0) return; - const wantedKeys = new Set(); + const wantedRefs = new Map(); for (const workspace of projectWorkspaces) { - const key = upstreamKey( - workspace.upstreamOwner, - workspace.upstreamRepo, - workspace.upstreamBranch ?? workspace.branch, - ); - if (key) wantedKeys.add(key); + const upstreamOwner = workspace.upstreamOwner; + const upstreamRepo = workspace.upstreamRepo; + const upstreamBranch = workspace.upstreamBranch ?? workspace.branch; + const key = upstreamKey(upstreamOwner, upstreamRepo, upstreamBranch); + if (key && upstreamOwner && upstreamRepo) { + wantedRefs.set(key, { + owner: upstreamOwner, + repo: upstreamRepo, + branch: upstreamBranch, + }); + } } const keyToPullRequest = await this.fetchRepoPullRequests( projectId, repo, - wantedKeys, + wantedRefs, options, ); @@ -828,13 +846,20 @@ export class PullRequestRuntimeManager { return rowId; } - private async getCachedRepoPullRequests( + private async getCachedPullRequestByHead( repo: NormalizedRepoIdentity, + head: GitHubPullRequestHeadRef, options: { bypassCache?: boolean } = {}, - ): Promise { - const cacheKey = `${repo.owner.toLowerCase()}/${repo.name.toLowerCase()}`; + ): Promise { + const cacheKey = [ + repo.owner.toLowerCase(), + repo.name.toLowerCase(), + head.owner.toLowerCase(), + head.repo.toLowerCase(), + head.branch, + ].join("/"); if (!options.bypassCache) { - const cached = this.repoPullRequestCache.get(cacheKey); + const cached = this.pullRequestHeadCache.get(cacheKey); if ( cached && Date.now() - cached.fetchedAt < REPO_PULL_REQUEST_CACHE_TTL_MS @@ -845,62 +870,151 @@ export class PullRequestRuntimeManager { const fetchedAt = Date.now(); const promise = (async () => { - const octokit = await this.github(); - return fetchRepositoryPullRequests(octokit, { - owner: repo.owner, - name: repo.name, - }); + try { + return await fetchPullRequestByHeadFromGh( + this.execGh, + { + owner: repo.owner, + name: repo.name, + }, + head, + ); + } catch (ghError) { + console.warn( + "[host-service:pull-request-runtime] gh PR head lookup failed; falling back to Octokit", + { + owner: repo.owner, + name: repo.name, + head, + error: ghError, + }, + ); + const octokit = await this.github(); + return fetchPullRequestByHead( + octokit, + { + owner: repo.owner, + name: repo.name, + }, + head, + ); + } })(); // Observer to silence unhandledRejection warnings; real consumers // observe the rejection via their own await on the cached promise. promise.catch(() => {}); // Keep failed promises cached for the full TTL so subsequent polls - // share the rejection without firing new GraphQL calls. Evicting on + // share the rejection without firing new GitHub calls. Evicting on // every error caused a self-perpetuating storm under rate-limit / // abuse-detection responses: the failure invalidated the cache, the // next 20s tick retried, hit the same 403, and re-evicted. Network // blips heal at the next TTL boundary instead. - this.repoPullRequestCache.set(cacheKey, { promise, fetchedAt }); + this.pullRequestHeadCache.set(cacheKey, { promise, fetchedAt }); return promise; } private async fetchRepoPullRequests( projectId: string, repo: NormalizedRepoIdentity, - wantedKeys: Set, + wantedRefs: Map, options: { bypassCache?: boolean } = {}, ): Promise> { - if (wantedKeys.size === 0) return new Map(); - - const nodes = await this.getCachedRepoPullRequests(repo, options); - - const latestByKey = new Map(); + if (wantedRefs.size === 0) return new Map(); - for (const node of nodes) { - const key = upstreamKey( - node.headRepositoryOwner?.login ?? null, - node.headRepository?.name ?? null, - node.headRefName, - ); - if (!key || !wantedKeys.has(key)) continue; - const existing = latestByKey.get(key); - if ( - !existing || - new Date(node.updatedAt).getTime() > - new Date(existing.updatedAt).getTime() - ) { - latestByKey.set(key, node); - } - } + const latestByKey = new Map(); + await Promise.all( + Array.from(wantedRefs.entries()).map(async ([key, head]) => { + try { + const node = await this.getCachedPullRequestByHead( + repo, + head, + options, + ); + if (!node) return; + + const nodeKey = upstreamKey( + node.headRepositoryOwner?.login ?? null, + node.headRepository?.name ?? null, + node.headRefName, + ); + if (nodeKey === key) latestByKey.set(key, node); + } catch (error) { + console.warn( + "[host-service:pull-request-runtime] Failed to fetch PR by head", + { + projectId, + owner: repo.owner, + name: repo.name, + head, + error, + }, + ); + } + }), + ); const keyToRow = new Map(); const now = Date.now(); + const checksByNumber = new Map< + number, + Awaited> + >(); + const reviewDecisionByNumber = new Map< + number, + GitHubPullRequestReviewDecision + >(); + let octokitPromise: Promise | null = null; + await Promise.all( + Array.from(latestByKey.values()).map(async (node) => { + try { + const [reviewDecision, checks] = await Promise.all([ + fetchPullRequestReviewDecisionFromGh( + this.execGh, + repo, + node.number, + node.state, + ), + fetchPullRequestChecksFromGh(this.execGh, repo, node.headRefOid), + ]); + reviewDecisionByNumber.set(node.number, reviewDecision); + checksByNumber.set(node.number, checks); + } catch (ghError) { + try { + octokitPromise ??= this.github(); + const octokit = await octokitPromise; + const [reviewDecision, checks] = await Promise.all([ + fetchPullRequestReviewDecision( + octokit, + repo, + node.number, + node.state, + ), + fetchPullRequestChecks(octokit, repo, node.headRefOid), + ]); + reviewDecisionByNumber.set(node.number, reviewDecision); + checksByNumber.set(node.number, checks); + } catch (error) { + console.warn( + "[host-service:pull-request-runtime] Failed to fetch PR review/check state", + { + projectId, + owner: repo.owner, + name: repo.name, + prNumber: node.number, + ghError, + error, + }, + ); + checksByNumber.set(node.number, []); + } + } + }), + ); + for (const [key, node] of latestByKey) { const existing = this.findPullRequestRow(repo, node.number); - const checks = parseCheckContexts( - node.statusCheckRollup?.contexts?.nodes ?? [], - ); + const checks = parseCheckContexts(checksByNumber.get(node.number) ?? []); const rowId = this.upsertPullRequestRow({ existing, projectId, @@ -912,7 +1026,9 @@ export class PullRequestRuntimeManager { isDraft: node.isDraft, headBranch: node.headRefName, headSha: node.headRefOid, - reviewDecision: mapReviewDecision(node.reviewDecision), + reviewDecision: mapReviewDecision( + reviewDecisionByNumber.get(node.number) ?? null, + ), checksStatus: computeChecksStatus(checks), checksJson: JSON.stringify(checks), lastFetchedAt: now, diff --git a/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts b/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts new file mode 100644 index 00000000000..fe7c35d10ce --- /dev/null +++ b/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts @@ -0,0 +1,217 @@ +import { describe, expect, test } from "bun:test"; +import { + fetchPullRequestByHeadFromGh, + fetchPullRequestChecksFromGh, + fetchPullRequestReviewDecisionFromGh, +} from "./github-query"; + +interface GhCall { + args: string[]; +} + +function createExecGh(responses: unknown[]) { + const calls: GhCall[] = []; + const execGh = async (args: string[]) => { + calls.push({ args }); + const response = responses.shift(); + if (response instanceof Error) throw response; + return response; + }; + + return { calls, execGh }; +} + +describe("GitHub pull request REST queries", () => { + test("fetches only the PR matching the requested upstream head", async () => { + const { calls, execGh } = createExecGh([ + [ + { + number: 42, + title: "Fix sidebar", + html_url: "https://github.com/superset-sh/superset/pull/42", + state: "open", + draft: false, + merged_at: null, + updated_at: "2026-05-08T12:00:00Z", + head: { + ref: "fix/sidebar", + sha: "abc123", + repo: { + name: "superset", + owner: { login: "superset-sh" }, + }, + }, + base: { + repo: { + full_name: "superset-sh/superset", + }, + }, + }, + ], + ]); + + const result = await fetchPullRequestByHeadFromGh( + execGh, + { owner: "superset-sh", name: "superset" }, + { owner: "superset-sh", repo: "superset", branch: "fix/sidebar" }, + ); + + expect(result).toEqual({ + number: 42, + title: "Fix sidebar", + url: "https://github.com/superset-sh/superset/pull/42", + state: "OPEN", + isDraft: false, + headRefName: "fix/sidebar", + headRefOid: "abc123", + isCrossRepository: false, + headRepositoryOwner: { login: "superset-sh" }, + headRepository: { name: "superset" }, + updatedAt: "2026-05-08T12:00:00Z", + }); + expect(calls).toEqual([ + { + args: [ + "api", + "--method", + "GET", + "repos/superset-sh/superset/pulls", + "-f", + "state=all", + "-f", + "head=superset-sh:fix/sidebar", + "-f", + "sort=updated", + "-f", + "direction=desc", + "-f", + "per_page=1", + ], + }, + ]); + }); + + test("derives review decision from latest REST reviews by author", async () => { + const { calls, execGh } = createExecGh([ + [ + { + user: { login: "a" }, + state: "APPROVED", + submitted_at: "2026-05-08T12:00:00Z", + }, + { + user: { login: "a" }, + state: "CHANGES_REQUESTED", + submitted_at: "2026-05-08T12:05:00Z", + }, + ], + ]); + + const result = await fetchPullRequestReviewDecisionFromGh( + execGh, + { owner: "superset-sh", name: "superset" }, + 42, + "OPEN", + ); + + expect(result).toBe("CHANGES_REQUESTED"); + expect(calls).toEqual([ + { + args: [ + "api", + "--method", + "GET", + "repos/superset-sh/superset/pulls/42/reviews", + "-f", + "per_page=100", + ], + }, + ]); + }); + + test("keeps open PRs pending when no terminal review exists", async () => { + const { execGh } = createExecGh([[]]); + + const result = await fetchPullRequestReviewDecisionFromGh( + execGh, + { owner: "superset-sh", name: "superset" }, + 42, + "OPEN", + ); + + expect(result).toBe("REVIEW_REQUIRED"); + }); + + test("fetches check runs and commit statuses for the matched PR head SHA", async () => { + const { calls, execGh } = createExecGh([ + { + check_runs: [ + { + name: "Typecheck", + conclusion: "success", + details_url: "https://github.com/superset-sh/superset/actions/1", + status: "completed", + started_at: "2026-05-08T12:00:00Z", + completed_at: "2026-05-08T12:03:00Z", + }, + ], + }, + [ + { + context: "CodeRabbit", + state: "success", + target_url: "https://coderabbit.ai", + created_at: "2026-05-08T12:04:00Z", + }, + ], + ]); + + const result = await fetchPullRequestChecksFromGh( + execGh, + { owner: "superset-sh", name: "superset" }, + "abc123", + ); + + expect(result).toEqual([ + { + __typename: "CheckRun", + name: "Typecheck", + conclusion: "SUCCESS", + detailsUrl: "https://github.com/superset-sh/superset/actions/1", + status: "COMPLETED", + startedAt: "2026-05-08T12:00:00Z", + completedAt: "2026-05-08T12:03:00Z", + checkSuite: null, + }, + { + __typename: "StatusContext", + context: "CodeRabbit", + state: "SUCCESS", + targetUrl: "https://coderabbit.ai", + createdAt: "2026-05-08T12:04:00Z", + }, + ]); + expect(calls).toEqual([ + { + args: [ + "api", + "--method", + "GET", + "repos/superset-sh/superset/commits/abc123/check-runs", + "-f", + "per_page=100", + ], + }, + { + args: [ + "api", + "--method", + "GET", + "repos/superset-sh/superset/commits/abc123/statuses", + "-f", + "per_page=100", + ], + }, + ]); + }); +}); diff --git a/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts b/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts index 88dcd636d2e..a2c126b51c2 100644 --- a/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts +++ b/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts @@ -1,26 +1,336 @@ import type { Octokit } from "@octokit/rest"; -import { PULL_REQUESTS_QUERY } from "./query"; +import type { ExecGh } from "../../../../trpc/router/workspace-creation/utils/exec-gh"; import type { - GraphQLPullRequestNode, - PullRequestsGraphQLResult, + GitHubCheckContextNode, + GitHubPullRequestHeadRef, + GitHubPullRequestNode, + GitHubPullRequestReviewDecision, } from "./types"; -export async function fetchRepositoryPullRequests( +type PullRequestState = GitHubPullRequestNode["state"]; + +interface RestReview { + user?: { + login?: string | null; + } | null; + state?: string | null; + submitted_at?: string | null; +} + +interface RestCheckRun { + name?: string | null; + conclusion?: string | null; + details_url?: string | null; + html_url?: string | null; + status?: string | null; + started_at?: string | null; + completed_at?: string | null; +} + +interface RestCheckRunsResponse { + check_runs?: RestCheckRun[]; +} + +interface RestCommitStatus { + context?: string | null; + state?: string | null; + target_url?: string | null; + created_at?: string | null; +} + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null; +} + +function asArray(value: unknown): unknown[] { + return Array.isArray(value) ? value : []; +} + +function normalizePullRequestState( + state: string, + mergedAt: string | null | undefined, +): PullRequestState { + if (mergedAt) return "MERGED"; + return state.toLowerCase() === "closed" ? "CLOSED" : "OPEN"; +} + +function normalizePullRequest(raw: unknown): GitHubPullRequestNode | null { + if (!isRecord(raw) || !isRecord(raw.head)) return null; + const headRepo = isRecord(raw.head.repo) ? raw.head.repo : null; + const headRepoOwner = + headRepo && isRecord(headRepo.owner) ? headRepo.owner : null; + const headUser = isRecord(raw.head.user) ? raw.head.user : null; + const ownerLogin = headRepoOwner?.login ?? headUser?.login; + + if ( + typeof raw.number !== "number" || + typeof raw.title !== "string" || + typeof raw.html_url !== "string" || + typeof raw.state !== "string" || + typeof raw.head.ref !== "string" || + typeof raw.head.sha !== "string" || + typeof ownerLogin !== "string" + ) { + return null; + } + + const repoName = + headRepo && typeof headRepo.name === "string" ? headRepo.name : null; + const baseRepo = + isRecord(raw.base) && isRecord(raw.base.repo) ? raw.base.repo : null; + const baseFullName = + baseRepo && typeof baseRepo.full_name === "string" + ? baseRepo.full_name.toLowerCase() + : null; + const headFullName = repoName + ? `${ownerLogin}/${repoName}`.toLowerCase() + : null; + + return { + number: raw.number, + title: raw.title, + url: raw.html_url, + state: normalizePullRequestState( + raw.state, + typeof raw.merged_at === "string" ? raw.merged_at : null, + ), + isDraft: raw.draft === true, + headRefName: raw.head.ref, + headRefOid: raw.head.sha, + isCrossRepository: + Boolean(baseFullName && headFullName) && baseFullName !== headFullName, + headRepositoryOwner: { login: ownerLogin }, + headRepository: repoName ? { name: repoName } : null, + updatedAt: + typeof raw.updated_at === "string" + ? raw.updated_at + : new Date(0).toISOString(), + }; +} + +function mapReviewDecision( + rawReviews: unknown, + prState: PullRequestState, +): GitHubPullRequestReviewDecision { + const latestByAuthor = new Map(); + + for (const item of asArray(rawReviews)) { + if (!isRecord(item)) continue; + const review = item as RestReview; + const login = review.user?.login; + const state = review.state; + if (!login || !state || state === "COMMENTED" || state === "PENDING") { + continue; + } + + const existing = latestByAuthor.get(login); + if ( + !existing || + Date.parse(review.submitted_at ?? "") > + Date.parse(existing.submitted_at ?? "") + ) { + latestByAuthor.set(login, review); + } + } + + let hasApproval = false; + for (const review of latestByAuthor.values()) { + if (review.state === "CHANGES_REQUESTED") return "CHANGES_REQUESTED"; + if (review.state === "APPROVED") hasApproval = true; + } + + if (hasApproval) return "APPROVED"; + return prState === "OPEN" ? "REVIEW_REQUIRED" : null; +} + +function toCheckRunNode(raw: RestCheckRun): GitHubCheckContextNode | null { + if (!raw.name) return null; + + return { + __typename: "CheckRun", + name: raw.name, + conclusion: raw.conclusion?.toUpperCase() ?? null, + detailsUrl: raw.details_url ?? raw.html_url ?? null, + status: raw.status?.toUpperCase() ?? "UNKNOWN", + startedAt: raw.started_at ?? null, + completedAt: raw.completed_at ?? null, + checkSuite: null, + }; +} + +function toStatusContextNode( + raw: RestCommitStatus, +): GitHubCheckContextNode | null { + if (!raw.context || !raw.state) return null; + + return { + __typename: "StatusContext", + context: raw.context, + state: raw.state.toUpperCase(), + targetUrl: raw.target_url ?? null, + createdAt: raw.created_at ?? null, + }; +} + +export async function fetchPullRequestByHeadFromGh( + execGh: ExecGh, + repository: { + owner: string; + name: string; + }, + head: GitHubPullRequestHeadRef, +): Promise { + const raw = await execGh([ + "api", + "--method", + "GET", + `repos/${repository.owner}/${repository.name}/pulls`, + "-f", + "state=all", + "-f", + `head=${head.owner}:${head.branch}`, + "-f", + "sort=updated", + "-f", + "direction=desc", + "-f", + "per_page=1", + ]); + + return normalizePullRequest(asArray(raw)[0]); +} + +export async function fetchPullRequestByHead( + octokit: Octokit, + repository: { + owner: string; + name: string; + }, + head: GitHubPullRequestHeadRef, +): Promise { + const response = await octokit.rest.pulls.list({ + owner: repository.owner, + repo: repository.name, + state: "all", + head: `${head.owner}:${head.branch}`, + sort: "updated", + direction: "desc", + per_page: 1, + }); + + return normalizePullRequest(response.data[0]); +} + +export async function fetchPullRequestReviewDecisionFromGh( + execGh: ExecGh, + repository: { + owner: string; + name: string; + }, + number: number, + prState: PullRequestState, +): Promise { + const raw = await execGh([ + "api", + "--method", + "GET", + `repos/${repository.owner}/${repository.name}/pulls/${number}/reviews`, + "-f", + "per_page=100", + ]); + + return mapReviewDecision(raw, prState); +} + +export async function fetchPullRequestReviewDecision( + octokit: Octokit, + repository: { + owner: string; + name: string; + }, + number: number, + prState: PullRequestState, +): Promise { + const response = await octokit.rest.pulls.listReviews({ + owner: repository.owner, + repo: repository.name, + pull_number: number, + per_page: 100, + }); + + return mapReviewDecision(response.data, prState); +} + +export async function fetchPullRequestChecksFromGh( + execGh: ExecGh, + repository: { + owner: string; + name: string; + }, + headSha: string, +): Promise { + const [checkRunsRaw, statusesRaw] = await Promise.all([ + execGh([ + "api", + "--method", + "GET", + `repos/${repository.owner}/${repository.name}/commits/${headSha}/check-runs`, + "-f", + "per_page=100", + ]), + execGh([ + "api", + "--method", + "GET", + `repos/${repository.owner}/${repository.name}/commits/${headSha}/statuses`, + "-f", + "per_page=100", + ]), + ]); + const checkRuns = isRecord(checkRunsRaw) + ? asArray((checkRunsRaw as RestCheckRunsResponse).check_runs) + : []; + const statuses = asArray(statusesRaw); + + return [ + ...checkRuns.map((item) => toCheckRunNode(item as RestCheckRun)), + ...statuses.map((item) => toStatusContextNode(item as RestCommitStatus)), + ].filter( + (node): node is NonNullable => node !== null, + ); +} + +export async function fetchPullRequestChecks( octokit: Octokit, repository: { owner: string; name: string; }, -): Promise { - const result = await octokit.graphql( - PULL_REQUESTS_QUERY, - { + headSha: string, +): Promise { + const [checkRunsResponse, statusesResponse] = await Promise.all([ + octokit.rest.checks.listForRef({ owner: repository.owner, repo: repository.name, - }, - ); + ref: headSha, + per_page: 100, + }), + octokit.rest.repos.listCommitStatusesForRef({ + owner: repository.owner, + repo: repository.name, + ref: headSha, + per_page: 100, + }), + ]); - return (result.repository?.pullRequests?.nodes ?? []).filter( - (node): node is GraphQLPullRequestNode => node !== null, + return [ + ...checkRunsResponse.data.check_runs.map((item) => + toCheckRunNode(item as RestCheckRun), + ), + ...statusesResponse.data.map((item) => + toStatusContextNode(item as RestCommitStatus), + ), + ].filter( + (node): node is NonNullable => node !== null, ); } diff --git a/packages/host-service/src/runtime/pull-requests/utils/github-query/index.ts b/packages/host-service/src/runtime/pull-requests/utils/github-query/index.ts index 864c78a90b9..30806c77bda 100644 --- a/packages/host-service/src/runtime/pull-requests/utils/github-query/index.ts +++ b/packages/host-service/src/runtime/pull-requests/utils/github-query/index.ts @@ -1,5 +1,14 @@ -export { fetchRepositoryPullRequests } from "./github-query"; +export { + fetchPullRequestByHead, + fetchPullRequestByHeadFromGh, + fetchPullRequestChecks, + fetchPullRequestChecksFromGh, + fetchPullRequestReviewDecision, + fetchPullRequestReviewDecisionFromGh, +} from "./github-query"; export type { - GraphQLCheckContextNode, - GraphQLPullRequestNode, + GitHubCheckContextNode, + GitHubPullRequestHeadRef, + GitHubPullRequestNode, + GitHubPullRequestReviewDecision, } from "./types"; diff --git a/packages/host-service/src/runtime/pull-requests/utils/github-query/query.ts b/packages/host-service/src/runtime/pull-requests/utils/github-query/query.ts deleted file mode 100644 index bf579dae8d4..00000000000 --- a/packages/host-service/src/runtime/pull-requests/utils/github-query/query.ts +++ /dev/null @@ -1,48 +0,0 @@ -export const PULL_REQUESTS_QUERY = ` - query PullRequestsForSidebar($owner: String!, $repo: String!) { - repository(owner: $owner, name: $repo) { - pullRequests(first: 100, states: [OPEN, CLOSED, MERGED], orderBy: { field: UPDATED_AT, direction: DESC }) { - nodes { - number - title - url - state - isDraft - headRefName - headRefOid - isCrossRepository - headRepositoryOwner { login } - headRepository { name } - reviewDecision - updatedAt - statusCheckRollup { - contexts(first: 50) { - nodes { - __typename - ... on CheckRun { - name - conclusion - detailsUrl - status - startedAt - completedAt - checkSuite { - workflowRun { - databaseId - } - } - } - ... on StatusContext { - context - state - targetUrl - createdAt - } - } - } - } - } - } - } - } -`; diff --git a/packages/host-service/src/runtime/pull-requests/utils/github-query/types.ts b/packages/host-service/src/runtime/pull-requests/utils/github-query/types.ts index 02201ae66e4..b8bed72a74d 100644 --- a/packages/host-service/src/runtime/pull-requests/utils/github-query/types.ts +++ b/packages/host-service/src/runtime/pull-requests/utils/github-query/types.ts @@ -1,4 +1,4 @@ -export interface GraphQLCheckRunNode { +export interface GitHubCheckRunNode { __typename: "CheckRun"; name: string; conclusion: string | null; @@ -13,7 +13,7 @@ export interface GraphQLCheckRunNode { } | null; } -export interface GraphQLStatusContextNode { +export interface GitHubStatusContextNode { __typename: "StatusContext"; context: string; state: string; @@ -21,12 +21,12 @@ export interface GraphQLStatusContextNode { createdAt: string | null; } -export type GraphQLCheckContextNode = - | GraphQLCheckRunNode - | GraphQLStatusContextNode +export type GitHubCheckContextNode = + | GitHubCheckRunNode + | GitHubStatusContextNode | null; -export interface GraphQLPullRequestNode { +export interface GitHubPullRequestNode { number: number; title: string; url: string; @@ -37,19 +37,17 @@ export interface GraphQLPullRequestNode { isCrossRepository: boolean; headRepositoryOwner: { login: string } | null; headRepository: { name: string } | null; - reviewDecision: "APPROVED" | "CHANGES_REQUESTED" | "REVIEW_REQUIRED" | null; updatedAt: string; - statusCheckRollup: { - contexts: { - nodes: GraphQLCheckContextNode[]; - } | null; - } | null; } -export interface PullRequestsGraphQLResult { - repository?: { - pullRequests?: { - nodes?: Array; - }; - } | null; +export type GitHubPullRequestReviewDecision = + | "APPROVED" + | "CHANGES_REQUESTED" + | "REVIEW_REQUIRED" + | null; + +export interface GitHubPullRequestHeadRef { + owner: string; + repo: string; + branch: string; } diff --git a/packages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.ts b/packages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.ts index b3f3637e319..92f282e79c6 100644 --- a/packages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.ts +++ b/packages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.ts @@ -1,6 +1,7 @@ import type { - GraphQLCheckContextNode, - GraphQLPullRequestNode, + GitHubCheckContextNode, + GitHubPullRequestNode, + GitHubPullRequestReviewDecision, } from "../github-query"; export type PullRequestState = "open" | "draft" | "merged" | "closed"; @@ -19,7 +20,7 @@ export interface PullRequestCheck { } export function mapPullRequestState( - state: GraphQLPullRequestNode["state"], + state: GitHubPullRequestNode["state"], isDraft: boolean, ): PullRequestState { if (state === "MERGED") return "merged"; @@ -29,7 +30,7 @@ export function mapPullRequestState( } export function mapReviewDecision( - value: GraphQLPullRequestNode["reviewDecision"], + value: GitHubPullRequestReviewDecision, ): ReviewDecision { if (value === "APPROVED") return "approved"; if (value === "CHANGES_REQUESTED") return "changes_requested"; @@ -38,11 +39,11 @@ export function mapReviewDecision( } export function parseCheckContexts( - nodes: GraphQLCheckContextNode[], + nodes: GitHubCheckContextNode[], ): PullRequestCheck[] { const checks = nodes .filter( - (node): node is NonNullable => node !== null, + (node): node is NonNullable => node !== null, ) .map((node) => { if (node.__typename === "CheckRun") { @@ -171,7 +172,7 @@ function mapStatusContextState(state: string): CheckStatus { } function getCheckRunRecency( - node: Extract, + node: Extract, ): number { const workflowRunId = node.checkSuite?.workflowRun?.databaseId; if (typeof workflowRunId === "number") { @@ -188,7 +189,7 @@ function getCheckRunRecency( } function getStatusContextRecency( - node: Extract, + node: Extract, ): number { if (!node.createdAt) { return 0; From 65c11f3f5cc142ec30f1a32d6d187e6d52191bb6 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 9 May 2026 00:19:37 -0700 Subject: [PATCH 2/3] fix(host-service): preserve last-known PR detail on refresh failure and match head candidates exactly - When `gh`/Octokit fail on the per-PR review/checks lookup (but the head lookup succeeded), keep the previously-stored `reviewDecision` and `checks` instead of clearing them. Avoids the badge flapping to "no checks" / "review required" on transient GitHub failures. - REST `pulls?head=` can return multiple PRs sharing the same head ref (e.g. across forks or closed/open states). Fetch up to 10 candidates and match exactly on `(headOwner, headRepo, headRef)`, with case-insensitive owner/repo comparison, before normalizing. --- .../pull-requests/pull-requests.test.ts | 140 ++++++++++++++++-- .../runtime/pull-requests/pull-requests.ts | 12 +- .../utils/github-query/github-query.test.ts | 63 +++++++- .../utils/github-query/github-query.ts | 38 ++++- 4 files changed, 233 insertions(+), 20 deletions(-) diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts index 8cd5304e8d5..61d8c961fc9 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts @@ -1,6 +1,9 @@ import { describe, expect, test } from "bun:test"; import { pullRequests, workspaces } from "../../db/schema"; -import { PullRequestRuntimeManager } from "./pull-requests"; +import { + PullRequestRuntimeManager, + type PullRequestRuntimeManagerOptions, +} from "./pull-requests"; const PROJECT_ID = "project-1"; const WORKSPACE_ID = "workspace-1"; @@ -139,18 +142,27 @@ function createFakeDb(state: FakeState) { }; } -function createManager(state: FakeState) { +function createManager( + state: FakeState, + overrides: Partial< + Pick + > = {}, +) { return new PullRequestRuntimeManager({ db: createFakeDb(state) as never, - execGh: async () => { - throw new Error("gh should not be used for direct PR linking"); - }, + execGh: + overrides.execGh ?? + (async () => { + throw new Error("gh should not be used for direct PR linking"); + }), git: async () => { throw new Error("git should not be used when project metadata is set"); }, - github: async () => { - throw new Error("github should not be used for direct PR linking"); - }, + github: + overrides.github ?? + (async () => { + throw new Error("github should not be used for direct PR linking"); + }), gitWatcher: { onChanged: () => () => {} } as never, }); } @@ -212,7 +224,13 @@ describe("PullRequestRuntimeManager direct checkout PR linking", () => { expect(state.workspace.upstreamRepo).toBeNull(); expect(state.workspace.upstreamBranch).toBeNull(); - await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); + const originalWarn = console.warn; + console.warn = () => {}; + try { + await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); + } finally { + console.warn = originalWarn; + } expect(state.workspace.pullRequestId).toBe(prId); }); @@ -238,8 +256,110 @@ describe("PullRequestRuntimeManager direct checkout PR linking", () => { }); state.workspace.headSha = "def456"; - await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); + const originalWarn = console.warn; + console.warn = () => {}; + try { + await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); + } finally { + console.warn = originalWarn; + } expect(state.workspace.pullRequestId).toBeNull(); }); + + test("preserves last-known review and checks when detail refresh fails", async () => { + const state = makeState("fix/sidebar"); + state.workspace = { + ...state.workspace, + headSha: "abc123", + upstreamOwner: "fork-owner", + upstreamRepo: "fork-repo", + upstreamBranch: "fix/sidebar", + pullRequestId: "pr-existing", + }; + state.pullRequest = { + id: "pr-existing", + projectId: PROJECT_ID, + repoProvider: "github", + repoOwner: "base-owner", + repoName: "base-repo", + prNumber: 42, + url: "https://github.com/base-owner/base-repo/pull/42", + title: "Fix sidebar", + state: "open", + isDraft: false, + headBranch: "fix/sidebar", + headSha: "old-sha", + reviewDecision: "approved", + checksStatus: "success", + checksJson: JSON.stringify([ + { + name: "Typecheck", + status: "success", + url: "https://github.com/base-owner/base-repo/actions/1", + }, + ]), + lastFetchedAt: 1, + error: null, + createdAt: 1, + updatedAt: 1, + }; + const manager = createManager(state, { + execGh: async (args) => { + const path = args.find((arg) => arg.startsWith("repos/")); + if (path === "repos/base-owner/base-repo/pulls") { + return [ + { + number: 42, + title: "Fix sidebar updated", + html_url: "https://github.com/base-owner/base-repo/pull/42", + state: "open", + draft: false, + merged_at: null, + updated_at: "2026-05-08T12:00:00Z", + head: { + ref: "fix/sidebar", + sha: "abc123", + repo: { + name: "fork-repo", + owner: { login: "fork-owner" }, + }, + }, + base: { + repo: { + full_name: "base-owner/base-repo", + }, + }, + }, + ]; + } + + throw new Error("detail refresh unavailable"); + }, + github: async () => { + throw new Error("octokit unavailable"); + }, + }); + + const originalWarn = console.warn; + console.warn = () => {}; + try { + await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); + } finally { + console.warn = originalWarn; + } + + expect(state.workspace.pullRequestId).toBe("pr-existing"); + expect(state.pullRequest?.title).toBe("Fix sidebar updated"); + expect(state.pullRequest?.headSha).toBe("abc123"); + expect(state.pullRequest?.reviewDecision).toBe("approved"); + expect(state.pullRequest?.checksStatus).toBe("success"); + expect(JSON.parse(state.pullRequest?.checksJson ?? "[]")).toEqual([ + { + name: "Typecheck", + status: "success", + url: "https://github.com/base-owner/base-repo/actions/1", + }, + ]); + }); }); diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.ts index c1039fc77f4..f400563ddb3 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.ts @@ -1006,7 +1006,6 @@ export class PullRequestRuntimeManager { error, }, ); - checksByNumber.set(node.number, []); } } }), @@ -1014,7 +1013,12 @@ export class PullRequestRuntimeManager { for (const [key, node] of latestByKey) { const existing = this.findPullRequestRow(repo, node.number); - const checks = parseCheckContexts(checksByNumber.get(node.number) ?? []); + const checks = checksByNumber.has(node.number) + ? parseCheckContexts(checksByNumber.get(node.number) ?? []) + : parseChecksJson(existing?.checksJson ?? null); + const reviewDecision = reviewDecisionByNumber.has(node.number) + ? mapReviewDecision(reviewDecisionByNumber.get(node.number) ?? null) + : coerceReviewDecision(existing?.reviewDecision ?? null); const rowId = this.upsertPullRequestRow({ existing, projectId, @@ -1026,9 +1030,7 @@ export class PullRequestRuntimeManager { isDraft: node.isDraft, headBranch: node.headRefName, headSha: node.headRefOid, - reviewDecision: mapReviewDecision( - reviewDecisionByNumber.get(node.number) ?? null, - ), + reviewDecision, checksStatus: computeChecksStatus(checks), checksJson: JSON.stringify(checks), lastFetchedAt: now, diff --git a/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts b/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts index fe7c35d10ce..a97e2062f25 100644 --- a/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts +++ b/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts @@ -85,12 +85,73 @@ describe("GitHub pull request REST queries", () => { "-f", "direction=desc", "-f", - "per_page=1", + "per_page=10", ], }, ]); }); + test("filters REST head candidates by exact upstream repository", async () => { + const { execGh } = createExecGh([ + [ + { + number: 41, + title: "Wrong fork", + html_url: "https://github.com/superset-sh/superset/pull/41", + state: "open", + draft: false, + merged_at: null, + updated_at: "2026-05-08T12:05:00Z", + head: { + ref: "fix/sidebar", + sha: "wrong", + repo: { + name: "other-repo", + owner: { login: "fork-owner" }, + }, + }, + base: { + repo: { + full_name: "superset-sh/superset", + }, + }, + }, + { + number: 42, + title: "Right fork", + html_url: "https://github.com/superset-sh/superset/pull/42", + state: "open", + draft: false, + merged_at: null, + updated_at: "2026-05-08T12:00:00Z", + head: { + ref: "fix/sidebar", + sha: "abc123", + repo: { + name: "fork-repo", + owner: { login: "Fork-Owner" }, + }, + }, + base: { + repo: { + full_name: "superset-sh/superset", + }, + }, + }, + ], + ]); + + const result = await fetchPullRequestByHeadFromGh( + execGh, + { owner: "superset-sh", name: "superset" }, + { owner: "fork-owner", repo: "fork-repo", branch: "fix/sidebar" }, + ); + + expect(result?.number).toBe(42); + expect(result?.headRepositoryOwner?.login).toBe("Fork-Owner"); + expect(result?.headRepository?.name).toBe("fork-repo"); + }); + test("derives review decision from latest REST reviews by author", async () => { const { calls, execGh } = createExecGh([ [ diff --git a/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts b/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts index a2c126b51c2..ab1b33eb05b 100644 --- a/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts +++ b/packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts @@ -108,6 +108,36 @@ function normalizePullRequest(raw: unknown): GitHubPullRequestNode | null { }; } +function headKey( + owner: string | null | undefined, + repo: string | null | undefined, + branch: string, +): string | null { + if (!owner || !repo) return null; + // GitHub owner/repo names are case-insensitive; branch names are not. + return `${owner.toLowerCase()}/${repo.toLowerCase()}#${branch}`; +} + +function normalizePullRequestCandidates( + raw: unknown, + head: GitHubPullRequestHeadRef, +): GitHubPullRequestNode | null { + const requestedKey = headKey(head.owner, head.repo, head.branch); + return ( + asArray(raw) + .map((item) => normalizePullRequest(item)) + .find( + (node) => + node && + headKey( + node.headRepositoryOwner?.login, + node.headRepository?.name, + node.headRefName, + ) === requestedKey, + ) ?? null + ); +} + function mapReviewDecision( rawReviews: unknown, prState: PullRequestState, @@ -194,10 +224,10 @@ export async function fetchPullRequestByHeadFromGh( "-f", "direction=desc", "-f", - "per_page=1", + "per_page=10", ]); - return normalizePullRequest(asArray(raw)[0]); + return normalizePullRequestCandidates(raw, head); } export async function fetchPullRequestByHead( @@ -215,10 +245,10 @@ export async function fetchPullRequestByHead( head: `${head.owner}:${head.branch}`, sort: "updated", direction: "desc", - per_page: 1, + per_page: 10, }); - return normalizePullRequest(response.data[0]); + return normalizePullRequestCandidates(response.data, head); } export async function fetchPullRequestReviewDecisionFromGh( From 691267768f2b912cf453b43fa6553d26d9c39bf7 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Sat, 9 May 2026 14:28:41 -0700 Subject: [PATCH 3/3] test(host-service): drop unnecessary console.warn silencing in PR linking tests These two cases exercise the local-DB lookup path and never reach the GitHub fetch branches that emit warnings, so the wrappers were dead. --- .../runtime/pull-requests/pull-requests.test.ts | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts index 61d8c961fc9..43425970e36 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts @@ -224,13 +224,7 @@ describe("PullRequestRuntimeManager direct checkout PR linking", () => { expect(state.workspace.upstreamRepo).toBeNull(); expect(state.workspace.upstreamBranch).toBeNull(); - const originalWarn = console.warn; - console.warn = () => {}; - try { - await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); - } finally { - console.warn = originalWarn; - } + await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); expect(state.workspace.pullRequestId).toBe(prId); }); @@ -256,13 +250,7 @@ describe("PullRequestRuntimeManager direct checkout PR linking", () => { }); state.workspace.headSha = "def456"; - const originalWarn = console.warn; - console.warn = () => {}; - try { - await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); - } finally { - console.warn = originalWarn; - } + await manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]); expect(state.workspace.pullRequestId).toBeNull(); });