fix(host-service): replace bulky GraphQL PR query with targeted REST per-head lookups#4291
Merged
Conversation
…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
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces a repository-wide GraphQL PR query with per-head REST lookups (gh CLI first, Octokit fallback), adds GitHub* types and runtime normalization, implements per-head caching, wires execGh through the manager/app, and adds tests for REST helpers and failure-preservation behavior. ChangesREST-Based Pull Request Fetching and Per-Head Caching
Sequence Diagram(s)sequenceDiagram
participant Manager
participant ExecGh
participant Octokit
participant DB
Manager->>ExecGh: fetch PRs by head (gh api)
alt ExecGh returns PR
ExecGh-->>Manager: PR node
else ExecGh fails
Manager->>Octokit: octokit.rest pulls.list / listReviews / checks.listForRef
Octokit-->>Manager: PR node / reviews / checks
end
Manager->>DB: upsert pull_requests (use fetched reviewDecision/checks)
DB-->>Manager: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryReplaces the single repo-wide
Confidence Score: 3/5The core fix is sound but a partial failure mode in fetchRepoPullRequests can silently overwrite a previously valid review decision with null in the database. When both gh and Octokit fail for the secondary review/check calls, reviewDecisionByNumber has no entry for that PR number. The upsert still runs and unconditionally writes null to the reviewDecision column, clearing any previously stored approved or changes_requested value — a visible regression for users who hit transient API failures. The rest of the change is well-structured and the fallback pattern is consistent. packages/host-service/src/runtime/pull-requests/pull-requests.ts — specifically the innermost catch block in fetchRepoPullRequests and the reviewDecision line in the upsert loop.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/runtime/pull-requests/pull-requests.ts | Core runtime rewritten to use per-head REST lookups; contains a logic gap where a complete review/check API failure during upsert silently clears the stored review decision. |
| packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts | New REST helpers for head lookup, review decision derivation, and check/status normalization; unpaginated 100-item caps on reviews and check-runs are a minor concern for heavy PRs. |
| packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts | New test file covering REST normalization, review decision derivation, and check+status rollup mapping; good coverage of the happy-path shapes. |
| packages/host-service/src/runtime/pull-requests/utils/github-query/types.ts | Type renames from GraphQL to GitHub prefix; removes embedded review/check fields from the PR node type and lifts them to standalone types. |
| packages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.ts | Type annotation updates only; no logic changes. |
| packages/host-service/src/runtime/pull-requests/pull-requests.test.ts | Test scaffolding updated with a no-op execGh stub for the direct-PR-linking path. |
| packages/host-service/src/app.ts | Minimal wiring change to pass execGh into PullRequestRuntimeManager. |
Sequence Diagram
sequenceDiagram
participant PM as PullRequestRuntimeManager
participant GH as gh api
participant OK as Octokit
participant DB as SQLite DB
PM->>PM: Build wantedRefs per workspace head ref
loop For each head ref
PM->>GH: "GET /repos/owner/repo/pulls?head=owner:branch&per_page=1"
alt gh fails
PM->>OK: octokit.rest.pulls.list
end
PM->>PM: Cache result in pullRequestHeadCache (60s TTL)
end
loop For each PR node found
PM->>GH: GET /pulls/number/reviews
PM->>GH: GET /commits/sha/check-runs and /statuses
alt gh fails
PM->>OK: listReviews + checks.listForRef
end
end
PM->>DB: upsertPullRequestRow with state + reviewDecision + checksStatus
Comments Outside Diff (1)
-
packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts, line 817-827 (link)Review decision truncated at 100 items without pagination
Both the
ghand Octokit review-fetching paths requestper_page=100but perform no pagination. The GitHub REST reviews API returns reviews in chronological (oldest-first) order, so on a PR with more than 100 reviews the most recent reviews per author — the ones that determine the actual review decision — will silently be absent.mapReviewDecisionwould then derive the decision from stale older reviews, potentially showing "changes_requested" for an author who later approved. This is a low-probability edge case but the same applies to the check-runs endpoint (fetchPullRequestChecksFromGh/fetchPullRequestChecks) which is equally unpaginated.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts Line: 817-827 Comment: **Review decision truncated at 100 items without pagination** Both the `gh` and Octokit review-fetching paths request `per_page=100` but perform no pagination. The GitHub REST reviews API returns reviews in chronological (oldest-first) order, so on a PR with more than 100 reviews the most recent reviews per author — the ones that determine the actual review decision — will silently be absent. `mapReviewDecision` would then derive the decision from stale older reviews, potentially showing "changes_requested" for an author who later approved. This is a low-probability edge case but the same applies to the check-runs endpoint (`fetchPullRequestChecksFromGh` / `fetchPullRequestChecks`) which is equally unpaginated. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/host-service/src/runtime/pull-requests/pull-requests.ts:997-1010
**Stale review decision written on partial failure**
When both `gh` and Octokit fail for review/check fetching, `reviewDecisionByNumber` is never populated for that PR. The upsert loop then resolves `reviewDecisionByNumber.get(node.number) ?? null` to `null`, and `mapReviewDecision(null)` returns `null`. `upsertPullRequestRow` unconditionally overwrites the stored row with this `null` value — so a PR that previously showed "approved" in the sidebar will silently flip to "no review decision" after any transient API failure that affects only the secondary calls, even though the head-lookup succeeded and the row is being updated. The fix is to fall back to `coerceReviewDecision(existing?.reviewDecision ?? null)` when `reviewDecisionByNumber` has no entry for the PR number.
### Issue 2 of 2
packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts:817-827
**Review decision truncated at 100 items without pagination**
Both the `gh` and Octokit review-fetching paths request `per_page=100` but perform no pagination. The GitHub REST reviews API returns reviews in chronological (oldest-first) order, so on a PR with more than 100 reviews the most recent reviews per author — the ones that determine the actual review decision — will silently be absent. `mapReviewDecision` would then derive the decision from stale older reviews, potentially showing "changes_requested" for an author who later approved. This is a low-probability edge case but the same applies to the check-runs endpoint (`fetchPullRequestChecksFromGh` / `fetchPullRequestChecks`) which is equally unpaginated.
Reviews (1): Last reviewed commit: "fix(host-service): replace bulky GraphQL..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+997
to
+1010
| } 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, []); | ||
| } |
There was a problem hiding this comment.
When both gh and Octokit fail for review/check fetching, reviewDecisionByNumber is never populated for that PR. The upsert loop then resolves reviewDecisionByNumber.get(node.number) ?? null to null, and mapReviewDecision(null) returns null. upsertPullRequestRow unconditionally overwrites the stored row with this null value — so a PR that previously showed "approved" in the sidebar will silently flip to "no review decision" after any transient API failure that affects only the secondary calls, even though the head-lookup succeeded and the row is being updated. The fix is to fall back to coerceReviewDecision(existing?.reviewDecision ?? null) when reviewDecisionByNumber has no entry for the PR number.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/runtime/pull-requests/pull-requests.ts
Line: 997-1010
Comment:
**Stale review decision written on partial failure**
When both `gh` and Octokit fail for review/check fetching, `reviewDecisionByNumber` is never populated for that PR. The upsert loop then resolves `reviewDecisionByNumber.get(node.number) ?? null` to `null`, and `mapReviewDecision(null)` returns `null`. `upsertPullRequestRow` unconditionally overwrites the stored row with this `null` value — so a PR that previously showed "approved" in the sidebar will silently flip to "no review decision" after any transient API failure that affects only the secondary calls, even though the head-lookup succeeded and the row is being updated. The fix is to fall back to `coerceReviewDecision(existing?.reviewDecision ?? null)` when `reviewDecisionByNumber` has no entry for the PR number.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts (1)
24-217: 💤 Low valueTests look good; one optional gap.
The argument-list assertions are nice and tight, and they pin both the endpoint shape and the per_page/sort defaults. One easy-to-add case worth considering: a closed/merged PR with only an
APPROVEDreview (covers theprState !== "OPEN" → nullbranch inmapReviewDecision), since the current “no terminal review” test only exercises the OPEN→REVIEW_REQUIREDbranch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts` around lines 24 - 217, Add a test that covers the non-OPEN PR branch in mapReviewDecision by calling fetchPullRequestReviewDecisionFromGh with a closed or merged state (e.g., "CLOSED" or "MERGED") and a single REST review payload containing only an APPROVED review; assert the returned decision is null and that the execGh call used the same reviews endpoint ("repos/.../pulls/42/reviews") with per_page=100 so the behavior for prState !== "OPEN" is exercised and verified.packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts (1)
111-144: 💤 Low valueReview-decision derivation looks correct, with one tiny edge case.
The "latest non-COMMENTED/non-PENDING per author, then CHANGES_REQUESTED-wins-over-APPROVED" logic matches GitHub's documented semantics for everything except branch-protection "required reviewers" (which is unavoidable without GraphQL). One minor edge case at line 129:
Date.parse(review.submitted_at ?? "")isNaNwhen missing, andNaN > NaN/NaN > anyNumberare bothfalse, so a later review with a missingsubmitted_atwill not replace an earlier one even if order suggests it should. In practice GitHub always populatessubmitted_atfor non-PENDING reviews, so this is theoretical — flagging only because the comparison silently drops candidates rather than falling back to insertion order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts` around lines 111 - 144, The comparison using Date.parse(review.submitted_at ?? "") can produce NaN and silently prevent later reviews from replacing earlier ones; update mapReviewDecision to track the iteration index and compare reviews by numeric timestamp first (use Date.parse(...) and treat invalid parse as NaN) but break ties / fallback to the iteration sequence so a later array item wins when timestamps are equal/invalid. Concretely, while iterating asArray(rawReviews) keep a monotonic counter (e.g., seq) and when deciding to replace an existing review in latestByAuthor compare parsed timestamps and if either is NaN or timestamps are equal use the seq to prefer the later item; reference mapReviewDecision, latestByAuthor, and review.submitted_at to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 925-1013: The concurrent gh subprocess fan-out is unbounded
because Promise.all maps over wantedRefs and latestByKey directly; introduce a
concurrency limiter (e.g., p-limit(8) or a simple semaphore) and wrap the
per-item async work so only a fixed number run at once. Apply the limiter when
mapping Array.from(wantedRefs.entries()).map(...) that calls
getCachedPullRequestByHead and when mapping
Array.from(latestByKey.values()).map(...) that calls
fetchPullRequestReviewDecisionFromGh and fetchPullRequestChecksFromGh (and their
octokit fallbacks fetchPullRequestReviewDecision/fetchPullRequestChecks),
returning limiter(() => /* original async block */) so Promise.all awaits
bounded tasks; keep octokitPromise usage the same but ensure octokit creation
happens inside the limited tasks.
---
Nitpick comments:
In
`@packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts`:
- Around line 24-217: Add a test that covers the non-OPEN PR branch in
mapReviewDecision by calling fetchPullRequestReviewDecisionFromGh with a closed
or merged state (e.g., "CLOSED" or "MERGED") and a single REST review payload
containing only an APPROVED review; assert the returned decision is null and
that the execGh call used the same reviews endpoint
("repos/.../pulls/42/reviews") with per_page=100 so the behavior for prState !==
"OPEN" is exercised and verified.
In
`@packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts`:
- Around line 111-144: The comparison using Date.parse(review.submitted_at ??
"") can produce NaN and silently prevent later reviews from replacing earlier
ones; update mapReviewDecision to track the iteration index and compare reviews
by numeric timestamp first (use Date.parse(...) and treat invalid parse as NaN)
but break ties / fallback to the iteration sequence so a later array item wins
when timestamps are equal/invalid. Concretely, while iterating
asArray(rawReviews) keep a monotonic counter (e.g., seq) and when deciding to
replace an existing review in latestByAuthor compare parsed timestamps and if
either is NaN or timestamps are equal use the seq to prefer the later item;
reference mapReviewDecision, latestByAuthor, and review.submitted_at to locate
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0803bdab-ce62-4462-92fe-1fcf18d1891f
📒 Files selected for processing (9)
packages/host-service/src/app.tspackages/host-service/src/runtime/pull-requests/pull-requests.test.tspackages/host-service/src/runtime/pull-requests/pull-requests.tspackages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.tspackages/host-service/src/runtime/pull-requests/utils/github-query/github-query.tspackages/host-service/src/runtime/pull-requests/utils/github-query/index.tspackages/host-service/src/runtime/pull-requests/utils/github-query/query.tspackages/host-service/src/runtime/pull-requests/utils/github-query/types.tspackages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.ts
💤 Files with no reviewable changes (1)
- packages/host-service/src/runtime/pull-requests/utils/github-query/query.ts
Comment on lines
+925
to
+1013
| 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<string, { id: string }>(); | ||
| const now = Date.now(); | ||
|
|
||
| const checksByNumber = new Map< | ||
| number, | ||
| Awaited<ReturnType<typeof fetchPullRequestChecks>> | ||
| >(); | ||
| const reviewDecisionByNumber = new Map< | ||
| number, | ||
| GitHubPullRequestReviewDecision | ||
| >(); | ||
| let octokitPromise: Promise<Octokit> | 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, []); | ||
| } | ||
| } | ||
| }), | ||
| ); |
Contributor
There was a problem hiding this comment.
Bound the gh subprocess fan-out per refresh.
performProjectRefresh issues Promise.all over every wanted head (line 925) and then again over every matched PR (line 968), where each PR triggers three concurrent execGh calls (review + check-runs + statuses). For a project with N distinct upstream branches that's roughly N + 3N concurrent gh subprocess spawns per refresh tick, multiplied across projects on the safety-net interval. On a machine tracking many workspaces this can saturate the OS process table and trip GitHub secondary rate limits (the same class of failure the 60s TTL was designed to dampen, but at the gh-process layer instead of the HTTP layer).
Consider a small concurrency limiter (e.g., p-limit(8) — or a hand-rolled semaphore if you don't want to add a dep) wrapping the per-head and per-PR work, so the fan-out stays bounded regardless of project size.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts` around
lines 925 - 1013, The concurrent gh subprocess fan-out is unbounded because
Promise.all maps over wantedRefs and latestByKey directly; introduce a
concurrency limiter (e.g., p-limit(8) or a simple semaphore) and wrap the
per-item async work so only a fixed number run at once. Apply the limiter when
mapping Array.from(wantedRefs.entries()).map(...) that calls
getCachedPullRequestByHead and when mapping
Array.from(latestByKey.values()).map(...) that calls
fetchPullRequestReviewDecisionFromGh and fetchPullRequestChecksFromGh (and their
octokit fallbacks fetchPullRequestReviewDecision/fetchPullRequestChecks),
returning limiter(() => /* original async block */) so Promise.all awaits
bounded tasks; keep octokitPromise usage the same but ensure octokit creation
happens inside the limited tasks.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/runtime/pull-requests/pull-requests.ts">
<violation number="1" location="packages/host-service/src/runtime/pull-requests/pull-requests.ts:941">
P1: Per-head fetch errors are swallowed, which causes transient API failures to clear `workspace.pullRequestId` values during refresh.</violation>
<violation number="2" location="packages/host-service/src/runtime/pull-requests/pull-requests.ts:1030">
P1: On partial failure (both `gh` and Octokit fail for review/check fetching), `reviewDecisionByNumber` is never populated for the affected PR. The fallback `?? null` then feeds `null` into `mapReviewDecision`, which returns `null` and unconditionally overwrites a previously stored review decision (e.g., "approved") in the DB. This causes the sidebar badge to silently flip to "no review decision" after any transient API failure on the secondary calls, even though the head-lookup succeeded. Preserve the existing stored value when the map has no entry for the PR number.</violation>
</file>
<file name="packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts">
<violation number="1" location="packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts:197">
P1: Fetching only one PR for `head=owner:branch` can miss the correct head repository and produce false “no PR” results.</violation>
<violation number="2" location="packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts:254">
P2: Review decision derivation is incorrect for PRs with more than 100 reviews because only the first page is processed.</violation>
<violation number="3" location="packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts:312">
P2: Checks/status rollup can be wrong on commits with more than 100 check runs or statuses because pagination is not handled.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+941
to
+952
| } catch (error) { | ||
| console.warn( | ||
| "[host-service:pull-request-runtime] Failed to fetch PR by head", | ||
| { | ||
| projectId, | ||
| owner: repo.owner, | ||
| name: repo.name, | ||
| head, | ||
| error, | ||
| }, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
P1: Per-head fetch errors are swallowed, which causes transient API failures to clear workspace.pullRequestId values during refresh.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/runtime/pull-requests/pull-requests.ts, line 941:
<comment>Per-head fetch errors are swallowed, which causes transient API failures to clear `workspace.pullRequestId` values during refresh.</comment>
<file context>
@@ -845,62 +870,151 @@ export class PullRequestRuntimeManager {
+ node.headRefName,
+ );
+ if (nodeKey === key) latestByKey.set(key, node);
+ } catch (error) {
+ console.warn(
+ "[host-service:pull-request-runtime] Failed to fetch PR by head",
</file context>
Suggested change
| } catch (error) { | |
| console.warn( | |
| "[host-service:pull-request-runtime] Failed to fetch PR by head", | |
| { | |
| projectId, | |
| owner: repo.owner, | |
| name: repo.name, | |
| head, | |
| error, | |
| }, | |
| ); | |
| } | |
| } catch (error) { | |
| console.warn( | |
| "[host-service:pull-request-runtime] Failed to fetch PR by head", | |
| { | |
| projectId, | |
| owner: repo.owner, | |
| name: repo.name, | |
| head, | |
| error, | |
| }, | |
| ); | |
| throw error; | |
| } |
| number: number, | ||
| prState: PullRequestState, | ||
| ): Promise<GitHubPullRequestReviewDecision> { | ||
| const response = await octokit.rest.pulls.listReviews({ |
Contributor
There was a problem hiding this comment.
P2: Review decision derivation is incorrect for PRs with more than 100 reviews because only the first page is processed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts, line 254:
<comment>Review decision derivation is incorrect for PRs with more than 100 reviews because only the first page is processed.</comment>
<file context>
@@ -1,26 +1,336 @@
+ number: number,
+ prState: PullRequestState,
+): Promise<GitHubPullRequestReviewDecision> {
+ const response = await octokit.rest.pulls.listReviews({
+ owner: repository.owner,
+ repo: repository.name,
</file context>
| headSha: string, | ||
| ): Promise<GitHubCheckContextNode[]> { | ||
| const [checkRunsResponse, statusesResponse] = await Promise.all([ | ||
| octokit.rest.checks.listForRef({ |
Contributor
There was a problem hiding this comment.
P2: Checks/status rollup can be wrong on commits with more than 100 check runs or statuses because pagination is not handled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts, line 312:
<comment>Checks/status rollup can be wrong on commits with more than 100 check runs or statuses because pagination is not handled.</comment>
<file context>
@@ -1,26 +1,336 @@
+ headSha: string,
+): Promise<GitHubCheckContextNode[]> {
+ const [checkRunsResponse, statusesResponse] = await Promise.all([
+ octokit.rest.checks.listForRef({
owner: repository.owner,
repo: repository.name,
</file context>
…nd 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.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/host-service/src/runtime/pull-requests/pull-requests.test.ts (1)
227-233: 💤 Low valueConsole suppression pattern is acceptable.
The
console.warnsuppression keeps test output clean now thatrefreshPullRequestsByWorkspacesusesexecGh-based queries that may log warnings. The pattern correctly restores the original in afinallyblock.♻️ Optional: Extract a helper to reduce repetition
This pattern appears 3 times in the file. You could extract a helper like:
async function withSuppressedWarnings<T>(fn: () => Promise<T>): Promise<T> { const originalWarn = console.warn; console.warn = () => {}; try { return await fn(); } finally { console.warn = originalWarn; } }Then use it as:
await withSuppressedWarnings(() => manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/runtime/pull-requests/pull-requests.test.ts` around lines 227 - 233, Extract a small test helper to DRY the console.warn suppression: add an exported or file-scoped async function withSuppressedWarnings<T>(fn: () => Promise<T>): Promise<T> that saves console.warn, replaces it with a no-op, awaits fn() in a try block and restores the original console.warn in finally; then replace the three inline suppression blocks (calls around manager.refreshPullRequestsByWorkspaces and similar calls) with await withSuppressedWarnings(() => manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID])) (or the corresponding call) so each site uses the new helper and restoration logic is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/host-service/src/runtime/pull-requests/pull-requests.test.ts`:
- Around line 227-233: Extract a small test helper to DRY the console.warn
suppression: add an exported or file-scoped async function
withSuppressedWarnings<T>(fn: () => Promise<T>): Promise<T> that saves
console.warn, replaces it with a no-op, awaits fn() in a try block and restores
the original console.warn in finally; then replace the three inline suppression
blocks (calls around manager.refreshPullRequestsByWorkspaces and similar calls)
with await withSuppressedWarnings(() =>
manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID])) (or the corresponding
call) so each site uses the new helper and restoration logic is centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a52da55d-7283-4af6-a454-b590909cf3ea
📒 Files selected for processing (4)
packages/host-service/src/runtime/pull-requests/pull-requests.test.tspackages/host-service/src/runtime/pull-requests/pull-requests.tspackages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.tspackages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.test.ts
- packages/host-service/src/runtime/pull-requests/pull-requests.ts
- packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.ts
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/runtime/pull-requests/pull-requests.ts">
<violation number="1" location="packages/host-service/src/runtime/pull-requests/pull-requests.ts:1016">
P1: When review/check fetch fails, this fallback can attach stale checks/review data to a new `headSha`, causing incorrect PR badge/status for the current commit.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+1016
to
+1021
| 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); |
Contributor
There was a problem hiding this comment.
P1: When review/check fetch fails, this fallback can attach stale checks/review data to a new headSha, causing incorrect PR badge/status for the current commit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/runtime/pull-requests/pull-requests.ts, line 1016:
<comment>When review/check fetch fails, this fallback can attach stale checks/review data to a new `headSha`, causing incorrect PR badge/status for the current commit.</comment>
<file context>
@@ -1006,15 +1006,19 @@ 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);
</file context>
Suggested change
| 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 sameHead = | |
| existing?.headSha.toLowerCase() === node.headRefOid.toLowerCase(); | |
| const checks = checksByNumber.has(node.number) | |
| ? parseCheckContexts(checksByNumber.get(node.number) ?? []) | |
| : sameHead | |
| ? parseChecksJson(existing?.checksJson ?? null) | |
| : []; | |
| const reviewDecision = reviewDecisionByNumber.has(node.number) | |
| ? mapReviewDecision(reviewDecisionByNumber.get(node.number) ?? null) | |
| : sameHead | |
| ? coerceReviewDecision(existing?.reviewDecision ?? null) | |
| : coerceReviewDecision(null); |
…king 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4246
Summary
PullRequestsForSidebarGraphQL query (up to 100 PRs +statusCheckRollupper PR in one roundtrip) with targeted REST calls scoped to the upstream branch each workspace actually tracks.gh apifirst (reuses the host-service auth path already proven for workspace creation) and fall back to Octokit on failure.(repo, headOwner, headRepo, branch)so multiple workspaces tracking the same upstream branch still collapse to one call per TTL window.(headOwner, headRepo, headRef)so we don't pick the wrong PR when GitHub returns multiple matches.Why / Context
On large repos with heavy CI, the GraphQL query times out at GitHub's edge with HTTP 504 (~11s) because materializing
statusCheckRollup.contexts(first: 50)across 100 PRs in one query exceeds GitHub's deadline. The failed promise is intentionally cached for the full 60s TTL to avoid rate-limit storms — so once the query starts failing, the sidebar PR badge never appears for any workspace until a restart, and even then it fails again on the next refresh.Issue #4246 captured this with concrete repro: 212 of 214
/graphqlrequests returned 504, 2 returned 500, zero 200s. Reducing the query topullRequests(first: 20, states: [OPEN])withoutstatusCheckRollupreturned 200 in ~660ms — confirming the rollup materialization is the cause.How It Works
fetchPullRequestByHead—GET /repos/{owner}/{repo}/pulls?head={ownerLogin}:{branch}&state=all&sort=updated&direction=desc&per_page=10. Targets the workspace's head ref instead of paginating the repo. Multiple candidates are filtered to the exact(headOwner, headRepo, headRef)match (case-insensitive on owner/repo, case-sensitive on branch).fetchPullRequestReviewDecision—GET /repos/{owner}/{repo}/pulls/{number}/reviews, then derive the decision client-side from the latest non-COMMENTED/PENDING review per author (matches the GraphQLreviewDecisionsemantics).fetchPullRequestChecks—GET /commits/{sha}/check-runsandGET /commits/{sha}/statusesin parallel; normalize into the sameGitHubCheckContextNodeshape the existingparseCheckContextsmapper consumes, so downstream check rollup logic is unchanged.*FromGhvariant (primary, viaexecGh) and an Octokit variant (fallback). Onghfailure for the head lookup, we log and fall back to Octokit; onghfailure for review/checks, we lazily acquire the Octokit client once per refresh and retry both calls together.${owner}/${repo}to${owner}/${repo}/${headOwner}/${headRepo}/${branch}. The TTL behavior (failures cached for full TTL to prevent retry storms) is preserved.ghand Octokit fail for the per-PR detail refresh, we leavechecksByNumber/reviewDecisionByNumberun-set for that PR. The upsert then sourcesreviewDecisionandchecksfrom the existing DB row instead of writing empty values, so the badge stays at its last-known state until a refresh succeeds.Manual QA Checklist
Happy path
gh pr viewfor the same PR.isCrossRepositoryis set correctly.Large-repo / 504 regression
POST /graphql - 504.Cache + multi-workspace
Failure / fallback
ghfailure (e.g., revokegh auth) — Octokit fallback fires and the badge still resolves.Multi-candidate head match
Testing
bun run typecheck(host-service)bun run lintbun test packages/host-service/src/runtime/pull-requests/— 9 pass (REST normalization, exact head-candidate match, review-decision derivation, check + status rollup mapping, and a regression test that the row's review/checks are preserved when the detail refresh fails)Design Decisions
ghprimary, Octokit fallback:ghreuses the host-service auth path already proven for workspace creation and tends to behave better under flaky network conditions; Octokit covers the case whereghis missing or unauthenticated.Risks / Rollout
Summary by CodeRabbit
Refactor
Stability
Tests