From 21ba8ec30e02f0fe16cf6394abe8b87bf83e46f7 Mon Sep 17 00:00:00 2001 From: Dominic R Date: Mon, 14 Sep 2026 19:41:35 -0400 Subject: [PATCH 1/8] fix(web): speed up pull request previews and detail reads --- apps/server/src/auth/RpcAuthorization.ts | 1 + .../pullRequest/GitHubPullRequestCli.test.ts | 95 +++++++- .../src/pullRequest/GitHubPullRequestCli.ts | 220 ++++++++++-------- .../GitHubPullRequestProvider.test.ts | 48 ++++ .../pullRequest/GitHubPullRequestProvider.ts | 94 ++++---- .../src/pullRequest/PullRequestProvider.ts | 9 + .../pullRequest/PullRequestService.test.ts | 79 ++++++- .../src/pullRequest/PullRequestService.ts | 55 +++++ .../src/pullRequest/gitHubPullRequestJson.ts | 39 ++++ apps/server/src/ws.ts | 6 + apps/web/src/components/ChatMarkdown.tsx | 1 + .../pullRequest/PullRequestLinkPreview.tsx | 102 +++++--- .../src/state/pullRequestRouting.ts | 1 + .../src/state/pullRequests.test.ts | 78 +++++++ .../client-runtime/src/state/pullRequests.ts | 18 ++ packages/contracts/src/pullRequest.ts | 14 ++ packages/contracts/src/rpc.ts | 9 + 17 files changed, 695 insertions(+), 174 deletions(-) diff --git a/apps/server/src/auth/RpcAuthorization.ts b/apps/server/src/auth/RpcAuthorization.ts index 36371713a7a8..3e10564d759c 100644 --- a/apps/server/src/auth/RpcAuthorization.ts +++ b/apps/server/src/auth/RpcAuthorization.ts @@ -73,6 +73,7 @@ export const RPC_REQUIRED_SCOPES = { [WS_METHODS.pullRequestsStack]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsLinkedThreads]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsDetail]: AuthOrchestrationReadScope, + [WS_METHODS.pullRequestsPreview]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsActivity]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsThreadComments]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsDiffFileContents]: AuthOrchestrationReadScope, diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index 2f90c5b79553..09d0dd021524 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -255,6 +255,60 @@ it.effect( ); layer("GitHubPullRequestCli.layer", (it) => { + it.effect("loads the complete hover card with one GraphQL request", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + encodeJson({ + data: { + repository: { + pullRequest: { + number: 7, + title: "Fast previews", + url: "https://github.example/acme/web/pull/7", + state: "MERGED", + isDraft: false, + createdAt: "2026-07-01T00:00:00Z", + author: { + login: "octocat", + name: "Octo Cat", + avatarUrl: "https://github.example/avatar.png", + }, + }, + }, + }, + }), + ), + ), + ); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + const preview = yield* cli.getPullRequestPreview({ + cwd: "/w", + repository: "acme/web", + host: "github.example", + number: 7, + }); + expect(preview).toEqual({ + number: 7, + title: "Fast previews", + url: "https://github.example/acme/web/pull/7", + state: "merged", + isDraft: false, + createdAt: "2026-07-01T00:00:00Z", + author: { + login: "octocat", + name: "Octo Cat", + avatarUrl: "https://github.example/avatar.png", + }, + }); + expect(mockedExecute).toHaveBeenCalledTimes(1); + expect(callAt(0).args).toEqual( + expect.arrayContaining(["api", "graphql", "--hostname", "github.example"]), + ); + }), + ); + it.effect("coalesces concurrent identity verification for the same host and credential", () => Effect.gen(function* () { mockedExecute.mockImplementation((input) => @@ -1943,8 +1997,47 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); + it.effect("reads workflow runs and their pull request scope concurrently", () => + Effect.gen(function* () { + const headsStarted = yield* Deferred.make(); + const runsStarted = yield* Deferred.make(); + mockedExecute.mockImplementation(({ args }) => + args[0] === "pr" + ? Deferred.succeed(headsStarted, undefined).pipe( + Effect.andThen(Deferred.await(runsStarted)), + Effect.as( + output( + '[{"number":7,"headRefOid":"abc123","isCrossRepository":true,"headRepositoryOwner":{"login":"octocat"}}]', + ), + ), + ) + : Deferred.succeed(runsStarted, undefined).pipe( + Effect.andThen(Deferred.await(headsStarted)), + Effect.as( + output('[{"databaseId":10,"workflowName":"build","url":"https://example.com/10"}]'), + ), + ), + ); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const runs = yield* cli.listWorkflowRunsRequiringApproval({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + headSha: "abc123", + headBranch: "feat/page", + headRepositoryOwner: "octocat", + isCrossRepository: true, + }); + + expect(runs).toEqual([{ id: 10, name: "build", url: "https://example.com/10" }]); + }), + ); + it.effect("refuses workflow approval when one head belongs to several pull requests", () => Effect.gen(function* () { + mockedExecute.mockReturnValue(Effect.succeed(output("[]"))); mockedExecute.mockReturnValueOnce( Effect.succeed( output( @@ -1983,7 +2076,7 @@ layer("GitHubPullRequestCli.layer", (it) => { limit: 1_000, }); expect(error.detail).toContain("instead of uniquely matching #7"); - expect(mockedExecute).toHaveBeenCalledTimes(1); + expect(mockedExecute).toHaveBeenCalledTimes(2); }), ); diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index 1b2efa7ec70e..7b5ca1260bfa 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -29,6 +29,7 @@ import { type PullRequestLabelCandidateList, type PullRequestThreadCommentsResult, type PullRequestUpdateMethod, + type PullRequestPreview, } from "@t3tools/contracts"; import * as GitHubCli from "../sourceControl/GitHubCli.ts"; @@ -42,6 +43,8 @@ import { decodeActorAvatarsJson, decodePullRequestActivityJson, decodePullRequestDetailJson, + decodePullRequestPreviewJson, + PULL_REQUEST_PREVIEW_GRAPHQL_QUERY, decodePullRequestFilesJson, decodePullRequestHeadsJson, decodePullRequestListJson, @@ -498,6 +501,16 @@ export class GitHubPullRequestCli extends Context.Service< readonly number: number; }) => Effect.Effect; + readonly getPullRequestPreview: (input: { + readonly cwd: string; + readonly repository: string; + readonly host: string; + readonly number: number; + }) => Effect.Effect< + Omit, + GitHubPullRequestCliError + >; + readonly listWorkflowRunsRequiringApproval: (input: { readonly cwd: string; readonly repository: string; @@ -1483,71 +1496,74 @@ export const make = Effect.gen(function* () { }); const listWorkflowRunsRequiringApproval: GitHubPullRequestCli["Service"]["listWorkflowRunsRequiringApproval"] = (input) => - github - .execute({ - cwd: input.cwd, - args: [ - "pr", - "list", - ...repositoryArgs(input), - "--state", - "open", - "--head", - input.headBranch, - "--limit", - workflowApprovalProbeLimit, - "--json", - "number,headRefOid,isCrossRepository,headRepositoryOwner", - ], - }) - .pipe( - Effect.flatMap( - ( - result, - ): Effect.Effect< - GitHubPullRequestHead, - GitHubPullRequestReadError | GitHubWorkflowApprovalRefusedError - > => { - const decoded = decodePullRequestHeadsJson(result.stdout.trim()); - if (!Result.isSuccess(decoded)) { - return Effect.fail(workflowApprovalReadError(input.cwd, decoded.failure)); - } - const exactHeads = decoded.success.filter( - (pullRequest) => - pullRequest.headSha === input.headSha && - pullRequest.isCrossRepository === true && - pullRequest.headRepositoryOwner?.toLowerCase() === - input.headRepositoryOwner.toLowerCase(), - ); - if (decoded.success.length > workflowApprovalLimit) { - return Effect.fail( - new GitHubWorkflowApprovalRefusedError({ - command: "gh", - cwd: input.cwd, - number: input.number, - reason: "head-list-truncated", - observedCount: decoded.success.length, - limit: workflowApprovalLimit, - }), - ); - } - if (exactHeads.length !== 1 || exactHeads[0]?.number !== input.number) { - return Effect.fail( - new GitHubWorkflowApprovalRefusedError({ - command: "gh", - cwd: input.cwd, - number: input.number, - reason: "head-not-unique", - observedCount: exactHeads.length, - limit: workflowApprovalLimit, - }), - ); - } - return Effect.succeed(exactHeads[0]); - }, - ), - Effect.flatMap(() => - github.execute({ + Effect.all( + [ + github + .execute({ + cwd: input.cwd, + args: [ + "pr", + "list", + ...repositoryArgs(input), + "--state", + "open", + "--head", + input.headBranch, + "--limit", + workflowApprovalProbeLimit, + "--json", + "number,headRefOid,isCrossRepository,headRepositoryOwner", + ], + }) + .pipe( + Effect.flatMap( + ( + result, + ): Effect.Effect< + GitHubPullRequestHead, + GitHubPullRequestReadError | GitHubWorkflowApprovalRefusedError + > => { + const decoded = decodePullRequestHeadsJson(result.stdout.trim()); + if (!Result.isSuccess(decoded)) { + return Effect.fail(workflowApprovalReadError(input.cwd, decoded.failure)); + } + const exactHeads = decoded.success.filter( + (pullRequest) => + pullRequest.headSha === input.headSha && + pullRequest.isCrossRepository === true && + pullRequest.headRepositoryOwner?.toLowerCase() === + input.headRepositoryOwner.toLowerCase(), + ); + if (decoded.success.length > workflowApprovalLimit) { + return Effect.fail( + new GitHubWorkflowApprovalRefusedError({ + command: "gh", + cwd: input.cwd, + number: input.number, + reason: "head-list-truncated", + observedCount: decoded.success.length, + limit: workflowApprovalLimit, + }), + ); + } + if (exactHeads.length !== 1 || exactHeads[0]?.number !== input.number) { + return Effect.fail( + new GitHubWorkflowApprovalRefusedError({ + command: "gh", + cwd: input.cwd, + number: input.number, + reason: "head-not-unique", + observedCount: exactHeads.length, + limit: workflowApprovalLimit, + }), + ); + } + return Effect.succeed(exactHeads[0]); + }, + ), + ), + github + .execute({ cwd: input.cwd, args: [ "run", @@ -1566,34 +1582,37 @@ export const make = Effect.gen(function* () { "--json", "databaseId,workflowName,url", ], - }), - ), - Effect.flatMap( - ( - result, - ): Effect.Effect< - ReadonlyArray, - GitHubPullRequestReadError | GitHubWorkflowApprovalRefusedError - > => { - const decoded = decodeWorkflowRunApprovalsJson(result.stdout.trim()); - if (!Result.isSuccess(decoded)) { - return Effect.fail(workflowApprovalReadError(input.cwd, decoded.failure)); - } - return decoded.success.length > workflowApprovalLimit - ? Effect.fail( - new GitHubWorkflowApprovalRefusedError({ - command: "gh", - cwd: input.cwd, - number: input.number, - reason: "run-list-truncated", - observedCount: decoded.success.length, - limit: workflowApprovalLimit, - }), - ) - : Effect.succeed(decoded.success); - }, - ), - ); + }) + .pipe( + Effect.flatMap( + ( + result, + ): Effect.Effect< + ReadonlyArray, + GitHubPullRequestReadError | GitHubWorkflowApprovalRefusedError + > => { + const decoded = decodeWorkflowRunApprovalsJson(result.stdout.trim()); + if (!Result.isSuccess(decoded)) { + return Effect.fail(workflowApprovalReadError(input.cwd, decoded.failure)); + } + return decoded.success.length > workflowApprovalLimit + ? Effect.fail( + new GitHubWorkflowApprovalRefusedError({ + command: "gh", + cwd: input.cwd, + number: input.number, + reason: "run-list-truncated", + observedCount: decoded.success.length, + limit: workflowApprovalLimit, + }), + ) + : Effect.succeed(decoded.success); + }, + ), + ), + ], + { concurrency: 2 }, + ).pipe(Effect.map(([, runs]) => runs)); return GitHubPullRequestCli.of({ withVerifiedCredential, @@ -1853,6 +1872,21 @@ export const make = Effect.gen(function* () { ), getPullRequestDetail, + getPullRequestPreview: (input) => { + const { owner, name } = parseRepositorySelector(input.repository); + return graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "getPullRequestPreview", + variables: [ + ["-f", `owner=${owner}`], + ["-f", `name=${name}`], + ["-F", `number=${input.number}`], + ], + query: PULL_REQUEST_PREVIEW_GRAPHQL_QUERY, + decode: decodePullRequestPreviewJson, + }); + }, listWorkflowRunsRequiringApproval, getPullRequestStack: (input) => { diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index c2571d91d563..b6e7c663752d 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "@effect/vitest"; +import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import * as Redacted from "effect/Redacted"; @@ -457,6 +458,53 @@ const openDetail = { commits: [], }; +it.effect("reads branch comparison and workflow approvals concurrently", () => + Effect.gen(function* () { + const comparisonStarted = yield* Deferred.make(); + const approvalsStarted = yield* Deferred.make(); + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + getPullRequestDetail: () => Effect.succeed(openDetail), + getPullRequestBaseComparison: () => + Deferred.succeed(comparisonStarted, undefined).pipe( + Effect.andThen(Deferred.await(approvalsStarted)), + Effect.as({ behindBy: 2, viewerCanUpdate: true }), + ), + listWorkflowRunsRequiringApproval: () => + Deferred.succeed(approvalsStarted, undefined).pipe( + Effect.andThen(Deferred.await(comparisonStarted)), + Effect.as([{ id: 123, name: "tests", url: "https://example.com/runs/123" }]), + ), + getRepositoryAccess: () => + Effect.succeed({ + canWrite: true, + mergeCapabilities: { merge: true, squash: true, rebase: true }, + }), + getViewerAccess: () => + Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }), + }), + ), + ); + + const detail = yield* provider.getChangeRequest({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }); + + expect(detail.behindBy).toBe(2); + expect(detail.workflowApprovalsRequired).toBe(1); + expect(detail.checks).toContainEqual({ + name: "tests", + status: "action-required", + description: "A maintainer must approve this workflow before it can run.", + url: "https://example.com/runs/123", + }); + }), +); + it.effect("does not classify same-repository gates as fork workflow approvals", () => Effect.gen(function* () { const provider = yield* make; diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index 1d003a970ee8..35a1d1657c27 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -336,59 +336,65 @@ export const make = Effect.gen(function* () { getChangeRequestStack: (input) => cli.getPullRequestStack(input).pipe(Effect.mapError(fail("getChangeRequestStack"))), + getChangeRequestPreview: (input) => + cli.getPullRequestPreview(input).pipe(Effect.mapError(fail("getChangeRequestPreview"))), + getChangeRequest: (input) => Effect.all( [ cli.getPullRequestDetail(input).pipe( Effect.flatMap((pullRequest) => - Effect.all({ - // Only an open pull request can be behind anything worth saying so about, and - // only one whose head repository is known can be compared at all. - comparison: - pullRequest.state !== "open" || pullRequest.headRepositoryOwner === null - ? Effect.succeed(null) - : cli - .getPullRequestBaseComparison({ - ...input, - headRef: `${pullRequest.headRepositoryOwner}:${pullRequest.headBranch}`, - }) - .pipe(Effect.orElseSucceed(() => null)), - // GitHub omits a fork workflow that has not been approved from the normal check - // rollup. Read the action-required runs by head revision so "all passed" cannot - // be shown while a whole workflow is still waiting to start. - workflowApprovals: - pullRequest.state !== "open" || pullRequest.isCrossRepository !== true - ? Effect.succeed({ - runs: [] as ReadonlyArray, - unavailable: false, - }) - : pullRequest.headSha == null || pullRequest.headRepositoryOwner == null + Effect.all( + { + // Only an open pull request can be behind anything worth saying so about, and + // only one whose head repository is known can be compared at all. + comparison: + pullRequest.state !== "open" || pullRequest.headRepositoryOwner === null + ? Effect.succeed(null) + : cli + .getPullRequestBaseComparison({ + ...input, + headRef: `${pullRequest.headRepositoryOwner}:${pullRequest.headBranch}`, + }) + .pipe(Effect.orElseSucceed(() => null)), + // GitHub omits a fork workflow that has not been approved from the normal check + // rollup. Read the action-required runs by head revision so "all passed" cannot + // be shown while a whole workflow is still waiting to start. + workflowApprovals: + pullRequest.state !== "open" || pullRequest.isCrossRepository !== true ? Effect.succeed({ runs: [] as ReadonlyArray, - unavailable: true, + unavailable: false, }) - : cli - .listWorkflowRunsRequiringApproval({ - ...input, - headSha: pullRequest.headSha, - headBranch: pullRequest.headBranch, - headRepositoryOwner: pullRequest.headRepositoryOwner, - isCrossRepository: true, + : pullRequest.headSha == null || pullRequest.headRepositoryOwner == null + ? Effect.succeed({ + runs: [] as ReadonlyArray, + unavailable: true, }) - .pipe( - Effect.matchEffect({ - onFailure: (error) => - error._tag === "GitHubCliRateLimitError" || - error._tag === "SourceControlRateLimitPausedError" - ? Effect.fail(error) - : Effect.succeed({ - runs: [] as ReadonlyArray, - unavailable: true, - }), - onSuccess: (runs) => Effect.succeed({ runs, unavailable: false }), - }), - ), - }).pipe(Effect.map((extra) => ({ pullRequest, ...extra }))), + : cli + .listWorkflowRunsRequiringApproval({ + ...input, + headSha: pullRequest.headSha, + headBranch: pullRequest.headBranch, + headRepositoryOwner: pullRequest.headRepositoryOwner, + isCrossRepository: true, + }) + .pipe( + Effect.matchEffect({ + onFailure: (error) => + error._tag === "GitHubCliRateLimitError" || + error._tag === "SourceControlRateLimitPausedError" + ? Effect.fail(error) + : Effect.succeed({ + runs: [] as ReadonlyArray, + unavailable: true, + }), + onSuccess: (runs) => Effect.succeed({ runs, unavailable: false }), + }), + ), + }, + { concurrency: 2 }, + ).pipe(Effect.map((extra) => ({ pullRequest, ...extra }))), ), ), getRepositoryAccess({ diff --git a/apps/server/src/pullRequest/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index 405eb6ae9e6c..9a79b5a614b4 100644 --- a/apps/server/src/pullRequest/PullRequestProvider.ts +++ b/apps/server/src/pullRequest/PullRequestProvider.ts @@ -30,6 +30,7 @@ import type { PullRequestReviewerKind, PullRequestLabelCandidateList, PullRequestState, + PullRequestPreview, PullRequestUpdateMethod, PullRequestViewerPermissions, SourceControlProviderKind, @@ -373,6 +374,14 @@ export interface PullRequestProviderApi { input: ProviderRepositoryRef & { readonly number: number }, ) => Effect.Effect; + /** Hosts without a narrow read use their existing detail response for hover cards. */ + readonly getChangeRequestPreview?: ( + input: ProviderRepositoryRef & { readonly number: number }, + ) => Effect.Effect< + Omit, + PullRequestProviderError + >; + /** * The cheap live fields used by linked threads. Optional because a provider without a narrow * endpoint can fall back to its full detail read at the service boundary. diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index c576101fa5a9..995a59479ad6 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -107,6 +107,82 @@ function hostedChangeRequest(body: string, additions = 1) { }; } +it.effect("caches narrow previews and invalidates them after refresh or mutation", () => + Effect.gen(function* () { + let reads = 0; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/w", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getViewer: () => Effect.die("preview must not read the viewer"), + getChangeRequestPreview: () => + Effect.sync(() => { + reads += 1; + return changeRequest(1, "2026-07-02T00:00:00Z"); + }), + }), + ], + }); + const ref = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const previews = yield* Effect.all([service.preview(ref), service.preview(ref)], { + concurrency: 2, + }); + assert.deepStrictEqual(previews[0], { + ...ref, + title: "Change request 1", + url: "https://host/pull/1", + author: { login: "octocat", name: null, avatarUrl: null }, + state: "open", + isDraft: false, + createdAt: "2026-07-01T00:00:00Z", + }); + assert.strictEqual(reads, 1); + yield* service.invalidate({ reference: ref }); + yield* service.preview(ref); + assert.strictEqual(reads, 2); + yield* service.runAction({ ...ref, action: "close" }); + yield* service.preview(ref); + assert.strictEqual(reads, 3); + yield* service.refreshAfterTurn(ref.projectId); + yield* service.preview(ref); + assert.strictEqual(reads, 4); + const error = yield* Effect.flip(service.preview({ ...ref, repository: "another/repo" })); + assert.strictEqual(error._tag, "PullRequestOperationError"); + assert.strictEqual(reads, 4); + }), +); + +it.effect("uses full detail for hosts without a narrow preview", () => + Effect.gen(function* () { + let reads = 0; + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "web", + workspaceRoot: "/w", + repository: "acme/web", + provider: "gitlab", + }), + ], + providers: [ + fakeProvider("gitlab", { + getChangeRequest: () => + Effect.sync(() => { + reads += 1; + return hostedChangeRequest("Description"); + }), + }), + ], + }); + const ref = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const preview = yield* service.preview(ref); + assert.strictEqual(preview.title, "Change request 1"); + assert.strictEqual(reads, 1); + assert.ok(!("body" in preview)); + }), +); + function unusable(provider: SourceControlProviderKind, reason: "missing-tool" | "unauthenticated") { return new PullRequestProviderError({ provider, @@ -3915,7 +3991,7 @@ it.effect("keeps routed reads separate when the GitHub account changes", () => it.effect("isolates routed caches for two credentials belonging to the same account", () => Effect.gen(function* () { - for (const operation of ["summary", "detail", "diff"] as const) { + for (const operation of ["summary", "detail", "diff", "preview"] as const) { let credential = "broad"; let calls = 0; const read = () => @@ -3945,6 +4021,7 @@ it.effect("isolates routed caches for two credentials belonging to the same acco read().pipe( Effect.as({ patch: "private patch", truncated: false, nextCursor: null }), ), + getChangeRequestPreview: read, }), ], }); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index d0d57f4ae8a6..49943de43e3a 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -31,6 +31,7 @@ import { type PullRequestCommentInput, type PullRequestCommentUpdateInput, type PullRequestDetail, + type PullRequestPreview, type PullRequestDiffFileContentsInput, type PullRequestDiffFileContentsResult, type PullRequestDiffStat, @@ -190,6 +191,9 @@ export class PullRequestService extends Context.Service< readonly subscribeRefreshes: Stream.Stream; readonly refreshAfterTurn: (projectId: ProjectId) => Effect.Effect; readonly detail: (input: PullRequestRef) => Effect.Effect; + readonly preview: ( + input: PullRequestRef, + ) => Effect.Effect; readonly activity: ( input: PullRequestRef, ) => Effect.Effect; @@ -513,6 +517,9 @@ function withRateLimitBackoff( listChangeRequestStats: wrap("listChangeRequestStats", api.listChangeRequestStats), }), getChangeRequest: wrap("getChangeRequest", api.getChangeRequest), + ...(api.getChangeRequestPreview === undefined + ? {} + : { getChangeRequestPreview: wrap("getChangeRequestPreview", api.getChangeRequestPreview) }), ...(api.getChangeRequestSummary === undefined ? {} : { @@ -1596,6 +1603,38 @@ export const make = Effect.gen(function* () { ), ); + const previewFields = (value: PullRequestPreview): PullRequestPreview => ({ + projectId: value.projectId, + repository: value.repository, + number: value.number, + title: value.title, + url: value.url, + author: value.author, + state: value.state, + isDraft: value.isDraft, + createdAt: value.createdAt, + }); + const previewUncached: PullRequestService["Service"]["preview"] = (input) => + requireProject(input).pipe( + Effect.flatMap((project) => + (project.api.getChangeRequestPreview ?? project.api.getChangeRequest)({ + cwd: project.project.workspaceRoot, + repository: project.repository, + host: project.host, + number: input.number, + }).pipe( + Effect.mapError(toPullRequestError("preview")), + Effect.map((value) => + previewFields({ + ...value, + projectId: project.project.id, + repository: project.repository, + }), + ), + ), + ), + ); + const activityUncached: PullRequestService["Service"]["activity"] = (input) => requireProject(input).pipe( Effect.flatMap((project) => @@ -2747,6 +2786,21 @@ export const make = Effect.gen(function* () { timeToLive: (exit) => (Exit.isSuccess(exit) ? DETAIL_CACHE_TTL : Duration.zero), }, ); + + const previewCache = yield* Cache.makeWith( + (key: string) => { + return previewUncached(refOfCacheKey(key)); + }, + { + capacity: DETAIL_CACHE_CAPACITY, + timeToLive: (exit) => (Exit.isSuccess(exit) ? DETAIL_CACHE_TTL : Duration.zero), + }, + ); + const preview: PullRequestService["Service"]["preview"] = (input) => { + const key = refCacheKey(input); + const held = input.allowStale === false ? undefined : lastGoodDetail.peek(key); + return held === undefined ? Cache.get(previewCache, key) : Effect.succeed(previewFields(held)); + }; const activity: PullRequestService["Service"]["activity"] = (input) => { const key = refCacheKey(input); return Cache.get(activityCache, key); @@ -2960,6 +3014,7 @@ export const make = Effect.gen(function* () { refreshAfterTurn, detail: credentialCached(detail), activity: credentialCached(activity), + preview: credentialCached(preview), threadComments, diff: credentialCached(diff), diffFileContents, diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.ts index 42fa0ba0e04f..0a93b1035987 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.ts @@ -5,6 +5,7 @@ import * as Schema from "effect/Schema"; import type { PullRequestStackMembership, PullRequestActor, + PullRequestPreview, PullRequestCheck, PullRequestCheckStatus, PullRequestChecksState, @@ -639,6 +640,44 @@ export const PULL_REQUEST_LIST_JSON_FIELDS = "number,title,url,author,headRefName,baseRefName,state,isDraft,mergeable,reviewDecision,additions,deletions,createdAt,updatedAt,mergedAt,reviewRequests,labels,statusCheckRollup"; export const PULL_REQUEST_DETAIL_JSON_FIELDS = `${PULL_REQUEST_LIST_JSON_FIELDS},body,changedFiles,closedAt,isCrossRepository,headRepositoryOwner,headRefOid,autoMergeRequest`; + +export const PULL_REQUEST_PREVIEW_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + number title url state isDraft createdAt + author { login avatarUrl ... on User { name } } + } + } +}`; + +const decodePullRequestPreview = decodeJsonResult( + Schema.Struct({ + data: Schema.Struct({ + repository: Schema.Struct({ + pullRequest: Schema.Struct({ + number: Schema.Int, + title: Schema.String, + url: Schema.String, + state: Schema.String, + isDraft: Schema.Boolean, + createdAt: Schema.String, + author: Schema.NullOr(RawActorSchema), + }), + }), + }), + }), +); + +export function decodePullRequestPreviewJson( + raw: string, +): Result.Result, DecodeFailure> { + return Result.map(decodePullRequestPreview(raw), ({ data }) => ({ + ...data.repository.pullRequest, + author: toActor(data.repository.pullRequest.author), + state: toState(data.repository.pullRequest), + })); +} + export const PULL_REQUEST_ACTIVITY_JSON_FIELDS = "author,comments,reviews,commits"; /** GitHub's own ceiling on a connection page, which is what both thread reads ask for. */ diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts index 707df77809cb..2c37d51a0234 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -2713,6 +2713,12 @@ const makeWsRpcLayer = ( "rpc.aggregate": "pull-requests", }, ), + [WS_METHODS.pullRequestsPreview]: (input) => + observeRpcEffect( + WS_METHODS.pullRequestsPreview, + withPullRequestViewer(input, pullRequests.preview(input)), + { "rpc.aggregate": "pull-requests" }, + ), [WS_METHODS.pullRequestsActivity]: (input) => observeRpcEffect( WS_METHODS.pullRequestsActivity, diff --git a/apps/web/src/components/ChatMarkdown.tsx b/apps/web/src/components/ChatMarkdown.tsx index b0209754b0d0..8fb0f79690df 100644 --- a/apps/web/src/components/ChatMarkdown.tsx +++ b/apps/web/src/components/ChatMarkdown.tsx @@ -2867,6 +2867,7 @@ const CHAT_MARKDOWN_COMPONENTS = { environmentId, input: { projectId: pullRequestProject.id, + host: pullRequestCandidate.host, repository: pullRequestProject.repositoryIdentity?.displayName ?? pullRequestCandidate.repository, diff --git a/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx b/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx index 36a4c8b15c1c..93f1bb5b1177 100644 --- a/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx +++ b/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx @@ -1,6 +1,12 @@ import { isAtomCommandInterrupted } from "@t3tools/client-runtime/state/runtime"; import type { EnvironmentId, PullRequestRef } from "@t3tools/contracts"; -import { cloneElement, useState, type ComponentPropsWithoutRef, type ReactElement } from "react"; +import { + cloneElement, + useEffect, + useState, + type ComponentPropsWithoutRef, + type ReactElement, +} from "react"; import { formatRelativeTimeLabel } from "~/timestampFormat"; import { pullRequestEnvironment } from "~/state/pullRequests"; @@ -34,9 +40,9 @@ export function PullRequestLinkPreview({ }) { const [open, setOpen] = useState(false); const [resolvingClick, setResolvingClick] = useState(false); - const detailQuery = useEnvironmentQuery( + const previewQuery = useEnvironmentQuery( open - ? pullRequestEnvironment.detail({ + ? pullRequestEnvironment.preview({ environmentId: target.environmentId, input: target.input, }) @@ -46,9 +52,33 @@ export function PullRequestLinkPreview({ reportFailure: false, reportDefect: false, }); + const readPreview = useAtomQueryRunner(pullRequestEnvironment.preview, { + reportFailure: false, + reportDefect: false, + }); + const { environmentId, input } = target; + // Keep the page warm while the smaller hover response is displayed. + useEffect(() => { + if (!open || previewQuery.data === null) return; + void readDetail({ + environmentId, + input, + }).catch(() => undefined); + }, [open, previewQuery.data, readDetail, environmentId, input]); + + const previewLink = cloneElement(link, { + onPointerEnter: (event) => { + link.props.onPointerEnter?.(event); + if (event.pointerType !== "touch") void readPreview(target).catch(() => undefined); + }, + onFocus: (event) => { + link.props.onFocus?.(event); + void readPreview(target).catch(() => undefined); + }, + }); const trigger = confirmBeforeOpen - ? cloneElement(link, { + ? cloneElement(previewLink, { onClick: (event) => { if (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) return; event.preventDefault(); @@ -56,7 +86,7 @@ export function PullRequestLinkPreview({ if (resolvingClick) return; setOpen(false); setResolvingClick(true); - void readDetail({ environmentId: target.environmentId, input: target.input }) + void readPreview(target) .then(async (result) => { if (isAtomCommandInterrupted(result)) return; if (result._tag === "Success" && onOpenPullRequest(result.value.url)) return; @@ -68,8 +98,8 @@ export function PullRequestLinkPreview({ .finally(() => setResolvingClick(false)); }, }) - : link; - const detail = detailQuery.data; + : previewLink; + const detail = previewQuery.data; const state = detail === null ? null @@ -84,36 +114,38 @@ export function PullRequestLinkPreview({ return ( - - {detail === null ? ( -

- {detailQuery.isPending ? "Loading pull request details…" : originalUrl} -

- ) : ( -
-
- {detail.repository} - #{detail.number} - · - {state === null ? null : ( - - - {state.label} - - )} -
-

- {detail.title} + {detail !== null || previewQuery.error !== null ? ( + + {detail === null ? ( +

+ {originalUrl}

-
- - {authorLabel} - · - opened {formatRelativeTimeLabel(detail.createdAt)} + ) : ( +
+
+ {detail.repository} + #{detail.number} + · + {state === null ? null : ( + + + {state.label} + + )} +
+

+ {detail.title} +

+
+ + {authorLabel} + · + opened {formatRelativeTimeLabel(detail.createdAt)} +
-
- )} - + )} + + ) : null} ); } diff --git a/packages/client-runtime/src/state/pullRequestRouting.ts b/packages/client-runtime/src/state/pullRequestRouting.ts index 555c870d04ef..f71154cd3d0a 100644 --- a/packages/client-runtime/src/state/pullRequestRouting.ts +++ b/packages/client-runtime/src/state/pullRequestRouting.ts @@ -29,6 +29,7 @@ const reads = new Set([ WS_METHODS.pullRequestsSummary, WS_METHODS.pullRequestsStack, WS_METHODS.pullRequestsDetail, + WS_METHODS.pullRequestsPreview, WS_METHODS.pullRequestsActivity, WS_METHODS.pullRequestsThreadComments, WS_METHODS.pullRequestsDiffFileContents, diff --git a/packages/client-runtime/src/state/pullRequests.test.ts b/packages/client-runtime/src/state/pullRequests.test.ts index 5fcf5fdf681e..3dc883e9cfac 100644 --- a/packages/client-runtime/src/state/pullRequests.test.ts +++ b/packages/client-runtime/src/state/pullRequests.test.ts @@ -808,6 +808,84 @@ it.effect("keeps concurrent diff file reads on different hosts separate", () => ), ); +it.effect("keeps hover previews fresh after edits and turns", () => + Effect.scoped( + Effect.gen(function* () { + const refreshEvents = yield* PubSub.unbounded(); + let title = "Original title"; + let reads = 0; + const read = (input: unknown) => + Effect.sync(() => { + expect(input).toEqual(reference); + reads++; + return { ...reference, title }; + }); + const reference = { + projectId: ProjectId.make("project-1"), + repository: "acme/web", + number: 1, + host: "github.example.com", + }; + const client = { + [WS_METHODS.pullRequestsSubscribeRefreshes]: () => Stream.fromPubSub(refreshEvents), + [WS_METHODS.pullRequestsPreview]: read, + [WS_METHODS.pullRequestsDetail]: read, + [WS_METHODS.pullRequestsUpdate]: (input: { title: string }) => + Effect.sync(() => { + title = input.title; + }), + [WS_METHODS.pullRequestsRunAction]: () => + Effect.sync(() => { + title = "Closed"; + }), + [WS_METHODS.pullRequestsInvalidate]: () => Effect.void, + } as unknown as WsRpcProtocolClient; + const { atoms, registry } = yield* makeTestRuntime(client); + const target = { environmentId: TARGET.environmentId, input: reference }; + const preview = atoms.preview(target); + const unmount = registry.mount(preview); + yield* Effect.addFinalizer(() => Effect.sync(unmount)); + expect( + (yield* AtomRegistry.getResult(registry, preview, { suspendOnWaiting: true })).title, + ).toBe(title); + yield* Effect.promise(() => executeAtomQuery(registry, preview)); + expect(reads).toBe(1); + const edited = yield* Effect.promise(() => + atoms.update.run(registry, { ...target, input: { ...reference, title: "Edited" } }), + ); + expect(AsyncResult.isSuccess(edited)).toBe(true); + expect( + (yield* AtomRegistry.getResult(registry, preview, { suspendOnWaiting: true })).title, + ).toBe("Edited"); + yield* Effect.promise(() => + atoms.runAction.run(registry, { ...target, input: { ...reference, action: "close" } }), + ); + expect( + (yield* AtomRegistry.getResult(registry, preview, { suspendOnWaiting: true })).title, + ).toBe("Closed"); + title = "Refreshed"; + yield* Effect.promise(() => + atoms.invalidate.run(registry, { + environmentId: target.environmentId, + input: { reference }, + }), + ); + expect( + (yield* AtomRegistry.getResult(registry, preview, { suspendOnWaiting: true })).title, + ).toBe("Refreshed"); + const refreshed = Latch.makeUnsafe(); + const stop = registry.subscribe(preview, (result) => { + if (AsyncResult.isSuccess(result) && result.value.title === "After turn") + refreshed.openUnsafe(); + }); + yield* Effect.addFinalizer(() => Effect.sync(stop)); + title = "After turn"; + yield* PubSub.publish(refreshEvents, 1); + yield* refreshed.await; + }), + ), +); + it.effect("refreshes pull request activity after a comment is updated", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/client-runtime/src/state/pullRequests.ts b/packages/client-runtime/src/state/pullRequests.ts index 1086066de682..533b151bb778 100644 --- a/packages/client-runtime/src/state/pullRequests.ts +++ b/packages/client-runtime/src/state/pullRequests.ts @@ -176,6 +176,15 @@ export function createPullRequestEnvironmentAtoms( refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), }), ); + const preview = writableQueryFamily( + createEnvironmentRpcQueryAtomFamily(runtime, { + label: "environment-data:pull-requests:preview", + tag: WS_METHODS.pullRequestsPreview, + execute: (input) => routedRequest(WS_METHODS.pullRequestsPreview, input), + staleTimeMs: 60_000, + refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), + }), + ); const labelCandidates = writableQueryFamily( createEnvironmentRpcQueryAtomFamily(runtime, { label: "environment-data:pull-requests:label-candidates", @@ -221,6 +230,7 @@ export function createPullRequestEnvironmentAtoms( refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }), }), detail, + preview, activity, threadComments: createEnvironmentRpcCommand(runtime, { label: "environment-data:pull-requests:thread-comments", @@ -274,6 +284,7 @@ export function createPullRequestEnvironmentAtoms( label: "environment-data:pull-requests:run-action", tag: WS_METHODS.pullRequestsRunAction, execute: (input) => routedRequest(WS_METHODS.pullRequestsRunAction, input), + onSuccess: (target, registry) => Effect.sync(() => registry.refresh(preview(target))), scheduler: commandScheduler, concurrency: serialPerEnvironment, }), @@ -281,6 +292,7 @@ export function createPullRequestEnvironmentAtoms( label: "environment-data:pull-requests:update", tag: WS_METHODS.pullRequestsUpdate, execute: (input) => routedRequest(WS_METHODS.pullRequestsUpdate, input), + onSuccess: (target, registry) => Effect.sync(() => registry.refresh(preview(target))), scheduler: commandScheduler, concurrency: serialPerEnvironment, }), @@ -464,6 +476,12 @@ export function createPullRequestEnvironmentAtoms( label: "environment-data:pull-requests:invalidate", tag: WS_METHODS.pullRequestsInvalidate, execute: (input) => routedRequest(WS_METHODS.pullRequestsInvalidate, input), + onSuccess: ({ environmentId, input }, registry) => + Effect.sync(() => { + if (input.reference !== undefined) { + registry.refresh(preview({ environmentId, input: input.reference })); + } + }), scheduler: commandScheduler, concurrency: serialPerEnvironment, }), diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index d2bd6a839d3d..6154a5d8c96f 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -688,6 +688,20 @@ export const PullRequestLinkedThreadsResult = Schema.Struct({ }); export type PullRequestLinkedThreadsResult = typeof PullRequestLinkedThreadsResult.Type; +/** The complete hover card, without checks, permissions, or branch comparisons. */ +export const PullRequestPreview = Schema.Struct({ + projectId: ProjectId, + repository: TrimmedNonEmptyString, + number: PositiveInt, + title: TrimmedNonEmptyString, + url: TrimmedNonEmptyString, + author: Schema.NullOr(PullRequestActor), + state: PullRequestState, + isDraft: Schema.Boolean, + createdAt: IsoDateTime, +}); +export type PullRequestPreview = typeof PullRequestPreview.Type; + /** * The small live shape a linked thread needs. Keeping it separate from detail means a sidebar * status check never loads permissions, repository settings, checks, or base comparison data. diff --git a/packages/contracts/src/rpc.ts b/packages/contracts/src/rpc.ts index 9096dd6e27f9..77805158b985 100644 --- a/packages/contracts/src/rpc.ts +++ b/packages/contracts/src/rpc.ts @@ -111,6 +111,7 @@ import { PullRequestCommentInput, PullRequestCommentUpdateInput, PullRequestDetail, + PullRequestPreview, PullRequestDiffFileContentsInput, PullRequestDiffFileContentsResult, PullRequestInvalidateInput, @@ -393,6 +394,7 @@ export const WS_METHODS = { pullRequestsStack: "pullRequests.stack", pullRequestsLinkedThreads: "pullRequests.linkedThreads", pullRequestsDetail: "pullRequests.detail", + pullRequestsPreview: "pullRequests.preview", pullRequestsActivity: "pullRequests.activity", pullRequestsThreadComments: "pullRequests.threadComments", pullRequestsDiffFileContents: "pullRequests.diffFileContents", @@ -733,6 +735,12 @@ const WsPullRequestsDetailRpc = Rpc.make(WS_METHODS.pullRequestsDetail, { error: PullRequestRpcError, }); +const WsPullRequestsPreviewRpc = Rpc.make(WS_METHODS.pullRequestsPreview, { + payload: PullRequestRef, + success: PullRequestPreview, + error: PullRequestRpcError, +}); + const WsPullRequestsActivityRpc = Rpc.make(WS_METHODS.pullRequestsActivity, { payload: PullRequestRef, success: PullRequestActivity, @@ -1402,6 +1410,7 @@ export const WsRpcGroup = RpcGroup.make( WsPullRequestsStackRpc, WsPullRequestsLinkedThreadsRpc, WsPullRequestsDetailRpc, + WsPullRequestsPreviewRpc, WsPullRequestsActivityRpc, WsPullRequestsThreadCommentsRpc, WsPullRequestsDiffFileContentsRpc, From 0239361c45f27b0a104545fe3731196e0eb4fb4b Mon Sep 17 00:00:00 2001 From: Dominic R Date: Mon, 14 Sep 2026 19:48:21 -0400 Subject: [PATCH 2/8] fix(web): expire held previews and preserve Forgejo ports --- .../pullRequest/PullRequestService.test.ts | 65 +++++++++++++++++++ .../src/pullRequest/PullRequestService.ts | 10 ++- apps/web/src/components/ChatMarkdown.tsx | 2 +- 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 995a59479ad6..ed48b1361067 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -152,6 +152,64 @@ it.effect("caches narrow previews and invalidates them after refresh or mutation }), ); +it.effect("reuses only unexpired detail for previews", () => + Effect.gen(function* () { + let previewReads = 0; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/w", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getChangeRequest: () => Effect.succeed(hostedChangeRequest("Description")), + getChangeRequestPreview: () => + Effect.sync(() => { + previewReads++; + return { + ...changeRequest(1, "2026-07-02T00:00:00Z"), + title: "Updated title", + state: "closed" as const, + }; + }), + }), + ], + }); + const ref = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + yield* service.detail(ref); + assert.strictEqual((yield* service.preview(ref)).title, "Change request 1"); + assert.strictEqual(previewReads, 0); + yield* TestClock.adjust("16 seconds"); + const preview = yield* service.preview(ref); + assert.strictEqual(preview.title, "Updated title"); + assert.strictEqual(preview.state, "closed"); + assert.strictEqual(previewReads, 1); + }), +); + +it.effect("does not wait for an in-flight detail read to display a preview", () => + Effect.gen(function* () { + const detailStarted = yield* Deferred.make(); + const releaseDetail = yield* Deferred.make(); + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/w", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getChangeRequest: () => + Deferred.succeed(detailStarted, undefined).pipe( + Effect.andThen(Deferred.await(releaseDetail)), + Effect.as(hostedChangeRequest("Description")), + ), + getChangeRequestPreview: () => Effect.succeed(changeRequest(1, "2026-07-02T00:00:00Z")), + }), + ], + }); + const ref = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const detail = yield* Effect.forkChild(service.detail(ref)); + yield* Deferred.await(detailStarted); + assert.strictEqual((yield* service.preview(ref)).title, "Change request 1"); + yield* Deferred.succeed(releaseDetail, undefined); + yield* Fiber.join(detail); + }), +); + it.effect("uses full detail for hosts without a narrow preview", () => Effect.gen(function* () { let reads = 0; @@ -1849,6 +1907,13 @@ it.effect("routes explicit Forgejo HTTP authorities through SSH checkouts after state: "open", }); assert.strictEqual(listed.viewers["code.example:3000"], "bilal"); + const preview = yield* service.preview({ + projectId: "ssh" as ProjectId, + host: "code.example:3000", + repository: "team/repo", + number: 42, + }); + assert.strictEqual(preview.number, 42); const detail = yield* service.detail({ projectId: "ssh" as ProjectId, host: "code.example:3000", diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 49943de43e3a..820e3872355f 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -2798,8 +2798,14 @@ export const make = Effect.gen(function* () { ); const preview: PullRequestService["Service"]["preview"] = (input) => { const key = refCacheKey(input); - const held = input.allowStale === false ? undefined : lastGoodDetail.peek(key); - return held === undefined ? Cache.get(previewCache, key) : Effect.succeed(previewFields(held)); + return Cache.getSuccess(detailCache, key).pipe( + Effect.flatMap( + Option.match({ + onNone: () => Cache.get(previewCache, key), + onSome: (detail) => Effect.succeed(previewFields(detail)), + }), + ), + ); }; const activity: PullRequestService["Service"]["activity"] = (input) => { const key = refCacheKey(input); diff --git a/apps/web/src/components/ChatMarkdown.tsx b/apps/web/src/components/ChatMarkdown.tsx index 8fb0f79690df..61e86f5e0f77 100644 --- a/apps/web/src/components/ChatMarkdown.tsx +++ b/apps/web/src/components/ChatMarkdown.tsx @@ -2867,7 +2867,7 @@ const CHAT_MARKDOWN_COMPONENTS = { environmentId, input: { projectId: pullRequestProject.id, - host: pullRequestCandidate.host, + host: pullRequestCandidate.authority ?? pullRequestCandidate.host, repository: pullRequestProject.repositoryIdentity?.displayName ?? pullRequestCandidate.repository, From 05efdd381bbf05ad6cecf82863942813fca59a90 Mon Sep 17 00:00:00 2001 From: Dominic R Date: Tue, 15 Sep 2026 19:07:24 -0400 Subject: [PATCH 3/8] test(server): preserve project-scoped preview caching after rebase --- .../pullRequest/PullRequestService.test.ts | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index ed48b1361067..0bc72fa8823b 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -210,6 +210,37 @@ it.effect("does not wait for an in-flight detail read to display a preview", () }), ); +it.effect("keeps previews warm when another project finishes a turn", () => + Effect.gen(function* () { + const reads: string[] = []; + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "web", workspaceRoot: "/w", repository: "acme/web" }), + project({ id: "p2", title: "docs", workspaceRoot: "/d", repository: "acme/docs" }), + ], + providers: [ + fakeProvider("github", { + getChangeRequestPreview: (input) => + Effect.sync(() => { + reads.push(input.repository); + return changeRequest(1, "2026-07-02T00:00:00Z"); + }), + }), + ], + }); + const web = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const docs = { projectId: "p2" as ProjectId, repository: "acme/docs", number: 1 }; + yield* service.preview(web); + yield* service.preview(docs); + yield* service.preview({ ...web, host: "github.com" }); + assert.deepStrictEqual(reads, ["acme/web", "acme/docs"]); + yield* service.refreshAfterTurn(web.projectId); + yield* service.preview(web); + yield* service.preview(docs); + assert.deepStrictEqual(reads, ["acme/web", "acme/docs", "acme/web"]); + }), +); + it.effect("uses full detail for hosts without a narrow preview", () => Effect.gen(function* () { let reads = 0; From 66c7fc470e9a7674b971f39fa0b19527129fa8fe Mon Sep 17 00:00:00 2001 From: Dominic R Date: Fri, 18 Sep 2026 09:16:10 -0400 Subject: [PATCH 4/8] fix(web): reduce GitHub requests for PR previews --- apps/server/scripts/measure-pr-preview.ts | 116 ++++++++++++++++++ .../pullRequest/GitHubPullRequestCli.test.ts | 61 ++++++++- .../pullRequest/PullRequestService.test.ts | 44 +++++++ .../pullRequest/PullRequestLinkPreview.tsx | 37 +----- 4 files changed, 223 insertions(+), 35 deletions(-) create mode 100644 apps/server/scripts/measure-pr-preview.ts diff --git a/apps/server/scripts/measure-pr-preview.ts b/apps/server/scripts/measure-pr-preview.ts new file mode 100644 index 000000000000..e0abe0e81a01 --- /dev/null +++ b/apps/server/scripts/measure-pr-preview.ts @@ -0,0 +1,116 @@ +// Run with: node apps/server/scripts/measure-pr-preview.ts owner/repo 123 456 +// Numbers form a session with shared repository-permission caches. Browser and +// service caches are excluded. Uses real GitHub reads, without a server or database. +// CLI-generated GraphQL +// queries are replayed with rateLimit.cost, outside the timed section, to measure +// their cost without confusing other applications' traffic with this process's. +import * as NodeServices from "@effect/platform-node/NodeServices"; +import * as Effect from "effect/Effect"; +import * as Console from "effect/Console"; +import * as Layer from "effect/Layer"; +import * as Schema from "effect/Schema"; + +import * as GitHubPullRequestCli from "../src/pullRequest/GitHubPullRequestCli.ts"; +import * as GitHubPullRequestProvider from "../src/pullRequest/GitHubPullRequestProvider.ts"; +import * as GitHubCli from "../src/sourceControl/GitHubCli.ts"; +import * as VcsProcess from "../src/vcs/VcsProcess.ts"; + +const [repository, ...numbers] = process.argv.slice(2); +if (!repository || numbers.length === 0 || numbers.some((number) => !/^\d+$/.test(number))) { + throw new Error("Usage: node apps/server/scripts/measure-pr-preview.ts owner/repo number..."); +} + +type Read = { graphqlRequests: number; restRequests: number; cost: number; debug: string }; +const reads: Read[] = []; +const measuredProcess = Layer.effect( + VcsProcess.VcsProcess, + Effect.gen(function* () { + const vcs = yield* VcsProcess.VcsProcess; + return VcsProcess.VcsProcess.of({ + run: (input) => + vcs.run({ ...input, env: { ...input.env, GH_DEBUG: input.env?.GH_DEBUG ?? "api" } }).pipe( + Effect.tap((output) => + Effect.sync(() => { + const graphqlRequests = [...output.stderr.matchAll(/^> POST \/graphql /gm)].length; + const requests = [...output.stderr.matchAll(/^> (?:GET|POST) /gm)].length; + const cost = /"rateLimit"\s*:\s*\{[^}]*"cost"\s*:\s*(\d+)/.exec(output.stdout)?.[1]; + reads.push({ + graphqlRequests, + restRequests: requests - graphqlRequests, + cost: Number(cost ?? 0), + debug: output.stderr, + }); + }), + ), + ), + }); + }), +).pipe(Layer.provide(VcsProcess.layer), Layer.provide(NodeServices.layer)); + +const services = GitHubPullRequestCli.layer.pipe( + Layer.provide(GitHubCli.layer), + Layer.provideMerge(measuredProcess), +); + +const decodeJson = Schema.decodeSync(Schema.fromJsonString(Schema.Unknown)); +const encodeJson = Schema.encodeSync(Schema.fromJsonString(Schema.Unknown)); +const graphqlCost = Effect.fn("measurePrPreview.graphqlCost")(function* (read: Read) { + if (read.cost || !read.graphqlRequests) return read.cost; + const vcs = yield* VcsProcess.VcsProcess; + let cost = 0; + let found = 0; + for (const match of read.debug.matchAll( + /GraphQL query:\n([\s\S]*?)\nGraphQL variables: (\{[^\n]*\})/g, + )) { + const query = match[1]!.replace(/(\bquery\b[^{]*\{)/, "$1 rateLimit { cost }"); + const output = yield* vcs.run({ + operation: "measurePrPreview.cost", + command: "gh", + cwd: process.cwd(), + args: ["api", "graphql", "--input", "-", "--jq", ".data.rateLimit.cost"], + stdin: encodeJson({ query, variables: decodeJson(match[2]!) }), + env: { GH_DEBUG: "" }, + }); + cost += Number(output.stdout.trim()); + found++; + } + if (found !== read.graphqlRequests) + throw new Error("Could not account for every GraphQL request"); + return cost; +}); + +for (const mode of ["detail", "preview"] as const) { + // Start each side cold, then retain the caches shared across PRs in one session. + const rows = await Effect.runPromise( + Effect.scoped( + Effect.gen(function* () { + const provider = yield* GitHubPullRequestProvider.make; + return yield* Effect.forEach(numbers.map(Number), (number) => + Effect.gen(function* () { + const input = { cwd: process.cwd(), repository, host: "github.com", number }; + reads.length = 0; + const start = performance.now(); + const value = yield* mode === "detail" + ? provider.getChangeRequest(input) + : provider.getChangeRequestPreview!(input); + const elapsedMs = Math.round(performance.now() - start); + const measured = [...reads]; + const costs = yield* Effect.forEach(measured, graphqlCost); + const points = costs.reduce((total, cost) => total + cost, 0); + return { + repository, + number, + mode, + state: value.state, + elapsedMs, + points, + graphqlRequests: measured.reduce((total, read) => total + read.graphqlRequests, 0), + restRequests: measured.reduce((total, read) => total + read.restRequests, 0), + }; + }), + ); + }), + ).pipe(Effect.provide(services)), + ); + for (const row of rows) await Effect.runPromise(Console.log(encodeJson(row))); +} diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index b55a5744a6b2..0d33db200ace 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -1,5 +1,6 @@ import { afterEach, assert, expect, it, vi } from "@effect/vitest"; import * as Effect from "effect/Effect"; +import * as Clock from "effect/Clock"; import * as Deferred from "effect/Deferred"; import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; @@ -33,7 +34,7 @@ const layer = it.layer( getPullRequest: mockedGetPullRequest, }), ), - Layer.provide(GitHubGraphQlBudget.layer), + Layer.provideMerge(GitHubGraphQlBudget.layer), ), ); @@ -255,6 +256,64 @@ it.effect( ); layer("GitHubPullRequestCli.layer", (it) => { + it.effect("admits only one concurrent preview above the reserve and resumes after reset", () => + Effect.gen(function* () { + const now = yield* Clock.currentTimeMillis; + const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; + const resetAt = "2099-08-13T14:00:00Z"; + yield* budget.observe( + "preview-budget.example", + encodeJson({ data: { rateLimit: { cost: 1, limit: 5_000, remaining: 501, resetAt } } }), + ); + mockedExecute.mockReturnValue( + Effect.succeed( + output( + encodeJson({ + data: { + repository: { + pullRequest: { + number: 7, + title: "Fast previews", + url: "https://preview-budget.example/acme/web/pull/7", + state: "OPEN", + isDraft: false, + createdAt: "2026-07-01T00:00:00Z", + author: null, + }, + }, + rateLimit: { cost: 1, limit: 5_000, remaining: 500, resetAt }, + }, + }), + ), + ), + ); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + const input = { + cwd: "/w", + repository: "acme/web", + host: "preview-budget.example", + number: 7, + }; + const results = yield* Effect.all( + Array.from({ length: 20 }, (_, index) => + cli.getPullRequestPreview({ ...input, number: index + 1 }).pipe(Effect.result), + ), + { concurrency: "unbounded" }, + ); + expect(results.filter((result) => result._tag === "Success")).toHaveLength(1); + for (const result of results) { + if (result._tag === "Failure") { + expect(result.failure._tag).toBe("SourceControlRateLimitPausedError"); + } + } + expect(mockedExecute).toHaveBeenCalledTimes(1); + yield* TestClock.setTime(Date.parse(resetAt)); + yield* cli.getPullRequestPreview(input); + expect(mockedExecute).toHaveBeenCalledTimes(2); + yield* TestClock.setTime(now); + }), + ); + it.effect("loads the complete hover card with one GraphQL request", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce( diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index e521ccc022c9..8e9262a08104 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -161,6 +161,50 @@ it.effect("caches narrow previews and invalidates them after refresh or mutation }), ); +it.effect("keeps cached previews available and pauses uncached previews until quota resets", () => + Effect.gen(function* () { + let reads = 0; + const retryAt = Date.parse("2099-08-13T14:00:00Z"); + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/w", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getChangeRequestPreview: ({ number }) => + Effect.gen(function* () { + reads++; + if (reads === 2) + return yield* new PullRequestProviderError({ + provider: "github", + operation: "getChangeRequestPreview", + reason: "rate-limited", + detail: "GitHub requests are paused until the rate limit resets.", + retryAt, + }); + return changeRequest(number, "2026-07-02T00:00:00Z"); + }), + }), + ], + }); + const ref = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + yield* service.preview(ref); + const limited = yield* Effect.flip(service.preview({ ...ref, number: 2 })); + assert.instanceOf(limited, PullRequestOperationError); + if (limited._tag !== "PullRequestOperationError") return; + assert.include(limited.detail, "paused"); + assert.strictEqual((yield* service.preview(ref)).number, 1); + for (const number of [2, 3, 4]) { + const paused = yield* Effect.flip(service.preview({ ...ref, number })); + assert.instanceOf(paused, PullRequestOperationError); + if (paused._tag !== "PullRequestOperationError") return; + assert.include(paused.detail, "paused"); + } + assert.strictEqual(reads, 2); + yield* TestClock.setTime(retryAt); + assert.strictEqual((yield* service.preview({ ...ref, number: 2 })).number, 2); + assert.strictEqual(reads, 3); + }), +); + it.effect("reuses only unexpired detail for previews", () => Effect.gen(function* () { let previewReads = 0; diff --git a/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx b/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx index 93f1bb5b1177..fd7a508a519f 100644 --- a/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx +++ b/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx @@ -1,12 +1,6 @@ import { isAtomCommandInterrupted } from "@t3tools/client-runtime/state/runtime"; import type { EnvironmentId, PullRequestRef } from "@t3tools/contracts"; -import { - cloneElement, - useEffect, - useState, - type ComponentPropsWithoutRef, - type ReactElement, -} from "react"; +import { cloneElement, useState, type ComponentPropsWithoutRef, type ReactElement } from "react"; import { formatRelativeTimeLabel } from "~/timestampFormat"; import { pullRequestEnvironment } from "~/state/pullRequests"; @@ -48,37 +42,12 @@ export function PullRequestLinkPreview({ }) : null, ); - const readDetail = useAtomQueryRunner(pullRequestEnvironment.detail, { - reportFailure: false, - reportDefect: false, - }); const readPreview = useAtomQueryRunner(pullRequestEnvironment.preview, { reportFailure: false, reportDefect: false, }); - const { environmentId, input } = target; - // Keep the page warm while the smaller hover response is displayed. - useEffect(() => { - if (!open || previewQuery.data === null) return; - void readDetail({ - environmentId, - input, - }).catch(() => undefined); - }, [open, previewQuery.data, readDetail, environmentId, input]); - - const previewLink = cloneElement(link, { - onPointerEnter: (event) => { - link.props.onPointerEnter?.(event); - if (event.pointerType !== "touch") void readPreview(target).catch(() => undefined); - }, - onFocus: (event) => { - link.props.onFocus?.(event); - void readPreview(target).catch(() => undefined); - }, - }); - const trigger = confirmBeforeOpen - ? cloneElement(previewLink, { + ? cloneElement(link, { onClick: (event) => { if (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) return; event.preventDefault(); @@ -98,7 +67,7 @@ export function PullRequestLinkPreview({ .finally(() => setResolvingClick(false)); }, }) - : previewLink; + : link; const detail = previewQuery.data; const state = detail === null From 854d10e52ebdc5ffc3a334e4b181618044435ac5 Mon Sep 17 00:00:00 2001 From: Dominic R Date: Fri, 18 Sep 2026 09:18:59 -0400 Subject: [PATCH 5/8] chore: register the PR preview measurement script --- knip.jsonc | 1 + 1 file changed, 1 insertion(+) diff --git a/knip.jsonc b/knip.jsonc index 0eae85a29acb..a06e0885a2f8 100644 --- a/knip.jsonc +++ b/knip.jsonc @@ -23,6 +23,7 @@ "src/claude-history-worker.ts!", "scripts/cli.ts", "scripts/evaluate-thread-titles.ts", + "scripts/measure-pr-preview.ts", "src/provider/testFixtures/*.mjs", ], // Keep the transitive Effect runtime pinned for standalone npm installs. From 5fbc595bb44eb9213a9d9c5a025d2f5b1e6beade Mon Sep 17 00:00:00 2001 From: Dominic R Date: Fri, 18 Sep 2026 09:20:51 -0400 Subject: [PATCH 6/8] fix(server): keep preview cost measurements on the same GitHub host --- apps/server/scripts/measure-pr-preview.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/apps/server/scripts/measure-pr-preview.ts b/apps/server/scripts/measure-pr-preview.ts index e0abe0e81a01..5cf15d9ed074 100644 --- a/apps/server/scripts/measure-pr-preview.ts +++ b/apps/server/scripts/measure-pr-preview.ts @@ -67,7 +67,16 @@ const graphqlCost = Effect.fn("measurePrPreview.graphqlCost")(function* (read: R operation: "measurePrPreview.cost", command: "gh", cwd: process.cwd(), - args: ["api", "graphql", "--input", "-", "--jq", ".data.rateLimit.cost"], + args: [ + "api", + "graphql", + "--hostname", + "github.com", + "--input", + "-", + "--jq", + ".data.rateLimit.cost", + ], stdin: encodeJson({ query, variables: decodeJson(match[2]!) }), env: { GH_DEBUG: "" }, }); From d8b9541cbe1107c9ab63d2991a450684eaa7f182 Mon Sep 17 00:00:00 2001 From: Dominic R Date: Fri, 18 Sep 2026 09:48:12 -0400 Subject: [PATCH 7/8] perf(server): combine PR permissions and merge settings --- apps/server/scripts/measure-pr-preview.ts | 5 +- .../pullRequest/GitHubPullRequestCli.test.ts | 43 +++------- .../src/pullRequest/GitHubPullRequestCli.ts | 39 +-------- .../GitHubPullRequestProvider.test.ts | 79 ++++++++++++------- .../pullRequest/GitHubPullRequestProvider.ts | 47 +---------- .../pullRequest/gitHubPullRequestJson.test.ts | 58 ++++++++++---- .../src/pullRequest/gitHubPullRequestJson.ts | 45 +++-------- 7 files changed, 125 insertions(+), 191 deletions(-) diff --git a/apps/server/scripts/measure-pr-preview.ts b/apps/server/scripts/measure-pr-preview.ts index 5cf15d9ed074..b7938edcac76 100644 --- a/apps/server/scripts/measure-pr-preview.ts +++ b/apps/server/scripts/measure-pr-preview.ts @@ -55,7 +55,10 @@ const services = GitHubPullRequestCli.layer.pipe( const decodeJson = Schema.decodeSync(Schema.fromJsonString(Schema.Unknown)); const encodeJson = Schema.encodeSync(Schema.fromJsonString(Schema.Unknown)); const graphqlCost = Effect.fn("measurePrPreview.graphqlCost")(function* (read: Read) { - if (read.cost || !read.graphqlRequests) return read.cost; + // The REST quota probe synthesizes rateLimit.cost for budget admission. + // It does not spend GraphQL points. + if (!read.graphqlRequests) return 0; + if (read.cost) return read.cost; const vcs = yield* VcsProcess.VcsProcess; let cost = 0; let found = 0; diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index 0d33db200ace..9ff6ccc5c6dd 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -3423,6 +3423,9 @@ layer("GitHubPullRequestCli.layer", (it) => { JSON.stringify({ data: { repository: { + mergeCommitAllowed: true, + squashMergeAllowed: false, + rebaseMergeAllowed: true, viewerPermission: "READ", pullRequest: { viewerCanUpdate: true, viewerDidAuthor: true }, }, @@ -3443,7 +3446,11 @@ layer("GitHubPullRequestCli.layer", (it) => { // One request, because both answers hang off the same repository object. assert.strictEqual(mockedExecute.mock.calls.length, 1); expect(callAt(0).args).toContain("number=7"); + expect(callAt(0).args.at(-1)).toContain( + "mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed", + ); expect(access).toEqual({ + mergeCapabilities: { merge: true, squash: false, rebase: true }, canWrite: false, canTriage: false, canUpdate: true, @@ -3585,6 +3592,9 @@ layer("GitHubPullRequestCli.layer", (it) => { JSON.stringify({ data: { repository: { + mergeCommitAllowed: true, + squashMergeAllowed: false, + rebaseMergeAllowed: true, viewerPermission: "READ", pullRequest: { viewerCanUpdate: true, viewerDidAuthor: true }, }, @@ -3612,6 +3622,7 @@ layer("GitHubPullRequestCli.layer", (it) => { assert.strictEqual(mockedExecute.mock.calls.length, 2); expect(access).toEqual({ + mergeCapabilities: { merge: true, squash: false, rebase: true }, canWrite: false, canTriage: false, canUpdate: true, @@ -3621,38 +3632,6 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); - it.effect("reads the viewer's role off the same call as the merge settings", () => - Effect.gen(function* () { - mockedExecute.mockReturnValue( - Effect.succeed( - output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ - mergeCommitAllowed: false, - squashMergeAllowed: true, - rebaseMergeAllowed: true, - viewerPermission: "WRITE", - }), - ), - ), - ); - const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; - - const access = yield* cli.getRepositoryAccess({ - cwd: "/w", - repository: "acme/web", - host: "github.com", - }); - - assert.strictEqual(mockedExecute.mock.calls.length, 1); - expect(callAt(0).args).toContain( - "mergeCommitAllowed,squashMergeAllowed,rebaseMergeAllowed,viewerPermission", - ); - assert.isTrue(access.canWrite); - expect(access.mergeCapabilities).toEqual({ merge: false, squash: true, rebase: true }); - }), - ); - it.effect("asks GitHub to review, naming the collection a request is added to", () => Effect.gen(function* () { mockedExecute.mockReturnValue(Effect.succeed(output("{}"))); diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index a74630452d92..517fdfc7468c 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -56,7 +56,6 @@ import { decodePullRequestStacksJson, decodePullRequestStatsJson, decodeReactionSubjectScopeJson, - decodeRepositoryAccessJson, decodeReviewerCandidatesJson, decodeLabelCandidatesJson, buildLabelRequestJson, @@ -81,7 +80,6 @@ import { REMOVE_REACTION_GRAPHQL_MUTATION, REVERT_PULL_REQUEST_GRAPHQL_MUTATION, gitHubReactionContent, - REPOSITORY_ACCESS_JSON_FIELDS, RESOLVE_REVIEW_THREAD_GRAPHQL_MUTATION, REVIEWER_CANDIDATES_GRAPHQL_QUERY, REVIEW_THREAD_COMMENTS_GRAPHQL_QUERY, @@ -652,13 +650,6 @@ export class GitHubPullRequestCli extends Context.Service< readonly cursor: string; }) => Effect.Effect; - /** One `gh repo view`, which answers what the repository allows and where the viewer stands. */ - readonly getRepositoryAccess: (input: { - readonly cwd: string; - readonly repository: string; - readonly host: string; - }) => Effect.Effect; - /** The viewer's standing on its own, for deciding a write without reading the whole detail. */ readonly getViewerAccess: (input: { readonly cwd: string; @@ -667,7 +658,7 @@ export class GitHubPullRequestCli extends Context.Service< readonly number: number; /** Manual action checks may use the quota held back from automatic reads. */ readonly allowReserve?: boolean | undefined; - }) => Effect.Effect; + }) => Effect.Effect; /** Who this pull request may be sent to, and who it has already been sent to. */ readonly listReviewerCandidates: (input: { @@ -2280,34 +2271,6 @@ export const make = Effect.gen(function* () { }); }, - getRepositoryAccess: (input) => - github - .execute({ - cwd: input.cwd, - args: [ - "repo", - "view", - `${input.host}/${input.repository}`, - "--json", - REPOSITORY_ACCESS_JSON_FIELDS, - ], - }) - .pipe( - Effect.flatMap((result) => { - const decoded = decodeRepositoryAccessJson(result.stdout.trim()); - return Result.isSuccess(decoded) - ? Effect.succeed(decoded.success) - : Effect.fail( - new GitHubPullRequestReadError({ - command: "gh", - cwd: input.cwd, - operation: "getRepositoryAccess", - cause: decoded.failure, - }), - ); - }), - ), - getViewerAccess: (input) => { const { owner, name } = parseRepositorySelector(input.repository); return graphqlRead({ diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index 1ed6037038e8..b666bb8bc3ab 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -306,10 +306,13 @@ describe("gitHubViewerPermissions", () => { comments: [], commits: [], }), - getRepositoryAccess: () => + getViewerAccess: () => GitHubCli.PinnedGitHubCredential.pipe( Effect.map((credential) => ({ canWrite: false, + canTriage: false, + canUpdate: true, + didAuthor: false, mergeCapabilities: { merge: true, squash: credential?.credentialFingerprint !== "restricted", @@ -317,13 +320,6 @@ describe("gitHubViewerPermissions", () => { }, })), ), - getViewerAccess: () => - Effect.succeed({ - canWrite: false, - canTriage: false, - canUpdate: true, - didAuthor: false, - }), }), ), ), @@ -413,13 +409,14 @@ describe("gitHubViewerPermissions", () => { ]), getPullRequestBaseComparison: () => Effect.succeed({ behindBy: 0, viewerCanUpdate: true }), - getRepositoryAccess: () => + getViewerAccess: () => Effect.succeed({ canWrite: true, + canTriage: true, + canUpdate: true, + didAuthor: false, mergeCapabilities: { merge: true, squash: true, rebase: true }, }), - getViewerAccess: () => - Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }), }), ), ), @@ -476,13 +473,14 @@ it.effect("reads branch comparison and workflow approvals concurrently", () => Effect.andThen(Deferred.await(comparisonStarted)), Effect.as([{ id: 123, name: "tests", url: "https://example.com/runs/123" }]), ), - getRepositoryAccess: () => + getViewerAccess: () => Effect.succeed({ canWrite: true, + canTriage: true, + canUpdate: true, + didAuthor: false, mergeCapabilities: { merge: true, squash: true, rebase: true }, }), - getViewerAccess: () => - Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }), }), ), ); @@ -524,13 +522,14 @@ it.effect("does not classify same-repository gates as fork workflow approvals", getPullRequestBaseComparison: () => Effect.succeed({ behindBy: 0, viewerCanUpdate: true }), listWorkflowRunsRequiringApproval: () => Effect.die("same-repository pull requests must not probe fork workflow approvals"), - getRepositoryAccess: () => + getViewerAccess: () => Effect.succeed({ canWrite: true, + canTriage: true, + canUpdate: true, + didAuthor: false, mergeCapabilities: { merge: true, squash: true, rebase: true }, }), - getViewerAccess: () => - Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }), }), ), ), @@ -571,13 +570,14 @@ it.effect("keeps an unsafe workflow approval scope visible as unknown", () => limit: 1_000, }), ), - getRepositoryAccess: () => + getViewerAccess: () => Effect.succeed({ canWrite: true, + canTriage: true, + canUpdate: true, + didAuthor: false, mergeCapabilities: { merge: true, squash: true, rebase: true }, }), - getViewerAccess: () => - Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }), }), ), ), @@ -610,13 +610,14 @@ it.effect("propagates workflow discovery rate limits", () => cause: new Error("rate limited"), }), ), - getRepositoryAccess: () => + getViewerAccess: () => Effect.succeed({ canWrite: true, + canTriage: true, + canUpdate: true, + didAuthor: false, mergeCapabilities: { merge: true, squash: true, rebase: true }, }), - getViewerAccess: () => - Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }), }), ), ), @@ -646,7 +647,13 @@ describe("getViewerPermissions", () => { getViewerAccess: () => Effect.sync(() => { accessReads++; - return { canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }; + return { + canWrite: true, + canTriage: true, + canUpdate: true, + didAuthor: false, + mergeCapabilities: { merge: true, squash: true, rebase: true }, + }; }), }), ), @@ -663,7 +670,13 @@ describe("getViewerPermissions", () => { getPullRequestDetail: () => Effect.succeed(openDetail), getPullRequestBaseComparison: () => comparison, getViewerAccess: () => - Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }), + Effect.succeed({ + canWrite: true, + canTriage: true, + canUpdate: true, + didAuthor: false, + mergeCapabilities: { merge: true, squash: true, rebase: true }, + }), }); it.effect("offers update-branch when the comparison grants it", () => @@ -709,7 +722,13 @@ describe("getViewerPermissions", () => { getViewerAccess: (input) => Effect.sync(() => { viewerAllowReserve = input.allowReserve; - return { canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }; + return { + canWrite: true, + canTriage: true, + canUpdate: true, + didAuthor: false, + mergeCapabilities: { merge: true, squash: true, rebase: true }, + }; }), }), ), @@ -744,7 +763,13 @@ describe("getViewerPermissions", () => { }), ), getViewerAccess: () => - Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }), + Effect.succeed({ + canWrite: true, + canTriage: true, + canUpdate: true, + didAuthor: false, + mergeCapabilities: { merge: true, squash: true, rebase: true }, + }), }), ), ), diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index 8ca33ac906b6..2c146dd90161 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -1,7 +1,4 @@ -import * as Cache from "effect/Cache"; -import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; -import * as Exit from "effect/Exit"; import type { PullRequestActor, PullRequestCapabilities, @@ -11,7 +8,6 @@ import type { } from "@t3tools/contracts"; import * as GitHubPullRequestCli from "./GitHubPullRequestCli.ts"; -import { PinnedGitHubCredential } from "../sourceControl/GitHubCli.ts"; import { PullRequestProviderError, type PullRequestProviderFailure, @@ -193,35 +189,6 @@ const rendersEmpty = (body: string): boolean => export const make = Effect.gen(function* () { const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; - const repositoryAccessCache = yield* Cache.makeWith( - (key: string) => { - const [cwd, repository, host] = JSON.parse(key) as [string, string, string]; - return cli.getRepositoryAccess({ cwd, repository, host }); - }, - { - capacity: 128, - timeToLive: (exit) => (Exit.isSuccess(exit) ? Duration.minutes(10) : Duration.zero), - }, - ); - const getRepositoryAccess = (input: { - readonly cwd: string; - readonly repository: string; - readonly host: string; - }) => - PinnedGitHubCredential.pipe( - Effect.flatMap((credential) => - Cache.get( - repositoryAccessCache, - JSON.stringify([ - input.cwd, - input.repository, - input.host, - credential?.credentialFingerprint ?? null, - ]), - ), - ), - ); - const fail = (operation: string) => (error: GitHubPullRequestCli.GitHubPullRequestCliError) => new PullRequestProviderError({ provider: "github", @@ -401,19 +368,13 @@ export const make = Effect.gen(function* () { ).pipe(Effect.map((extra) => ({ pullRequest, ...extra }))), ), ), - getRepositoryAccess({ - cwd: input.cwd, - repository: input.repository, - host: input.host, - }), - // A small permissions query replaces the deeply paginated review-thread walk on the - // core path. Writes ask again immediately before mutating, so this is presentation. + // Repository merge settings travel with the viewer permissions. cli.getViewerAccess(input), ], - { concurrency: 3 }, + { concurrency: 2 }, ).pipe( Effect.mapError(fail("getChangeRequest")), - Effect.map(([detail, repository, viewerAccess]): ProviderChangeRequestDetail => ({ + Effect.map(([detail, viewerAccess]): ProviderChangeRequestDetail => ({ ...detail.pullRequest, author: withAvatar(detail.pullRequest.author, new Map(), input.host), checks: withWorkflowApprovals( @@ -429,7 +390,7 @@ export const make = Effect.gen(function* () { name: null, avatarUrl: null, })), - mergeCapabilities: repository.mergeCapabilities, + mergeCapabilities: viewerAccess.mergeCapabilities, viewerPermissions: gitHubViewerPermissions({ ...viewerAccess, canUpdateBranch: detail.comparison?.viewerCanUpdate === true, diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts index 8100a167808e..4591ac0437bf 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts @@ -17,7 +17,6 @@ import { decodePullRequestSearchJson, decodePullRequestStacksJson, decodeLabelCandidatesJson, - decodeRepositoryAccessJson, decodeReviewerCandidatesJson, decodeReviewThreadCommentsJson, decodeReviewThreadsJson, @@ -810,31 +809,38 @@ describe("reaction decoding", () => { describe("repository access decoding", () => { const repositoryJson = (viewerPermission?: string | null) => JSON.stringify({ - mergeCommitAllowed: true, - squashMergeAllowed: false, - rebaseMergeAllowed: true, - ...(viewerPermission === undefined ? {} : { viewerPermission }), + data: { + repository: { + pullRequest: null, + mergeCommitAllowed: true, + squashMergeAllowed: false, + rebaseMergeAllowed: true, + ...(viewerPermission === undefined ? {} : { viewerPermission }), + }, + }, }); - it("reads the three settings gh reports", () => { + it("reads merge settings with viewer permissions", () => { expect( - expectSuccess(decodeRepositoryAccessJson(repositoryJson("ADMIN"))).mergeCapabilities, + expectSuccess(decodeViewerPermissionsJson(repositoryJson("ADMIN"))).mergeCapabilities, ).toEqual({ merge: true, squash: false, rebase: true }); }); it("fails rather than defaulting open when a setting is missing", () => { - const decoded = decodeRepositoryAccessJson(JSON.stringify({ mergeCommitAllowed: true })); + const decoded = decodeViewerPermissionsJson( + JSON.stringify({ data: { repository: { pullRequest: null, mergeCommitAllowed: true } } }), + ); expect(Result.isSuccess(decoded)).toBe(false); }); it("counts the roles that can push as write, and the ones that cannot as read", () => { for (const permission of ["ADMIN", "MAINTAIN", "WRITE"]) { - expect(expectSuccess(decodeRepositoryAccessJson(repositoryJson(permission))).canWrite).toBe( + expect(expectSuccess(decodeViewerPermissionsJson(repositoryJson(permission))).canWrite).toBe( true, ); } for (const permission of ["TRIAGE", "READ", "NONE"]) { - expect(expectSuccess(decodeRepositoryAccessJson(repositoryJson(permission))).canWrite).toBe( + expect(expectSuccess(decodeViewerPermissionsJson(repositoryJson(permission))).canWrite).toBe( false, ); } @@ -843,14 +849,23 @@ describe("repository access decoding", () => { it("withholds write where gh names no permission, which is not a standing it gave", () => { // The one place an unknown answer is not granted: a Merge button a reader cannot use wastes // the press, where a missing one still leaves the pull request open on its host. - expect(expectSuccess(decodeRepositoryAccessJson(repositoryJson())).canWrite).toBe(false); - expect(expectSuccess(decodeRepositoryAccessJson(repositoryJson(null))).canWrite).toBe(false); + expect(expectSuccess(decodeViewerPermissionsJson(repositoryJson())).canWrite).toBe(false); + expect(expectSuccess(decodeViewerPermissionsJson(repositoryJson(null))).canWrite).toBe(false); }); }); describe("viewer permission decoding", () => { const viewerJson = (repository: Record) => - JSON.stringify({ data: { repository } }); + JSON.stringify({ + data: { + repository: { + mergeCommitAllowed: true, + squashMergeAllowed: false, + rebaseMergeAllowed: true, + ...repository, + }, + }, + }); it("reads the repository's role and the pull request's own viewer fields together", () => { expect( @@ -862,7 +877,13 @@ describe("viewer permission decoding", () => { }), ), ), - ).toEqual({ canWrite: false, canTriage: false, canUpdate: true, didAuthor: true }); + ).toEqual({ + mergeCapabilities: { merge: true, squash: false, rebase: true }, + canWrite: false, + canTriage: false, + canUpdate: true, + didAuthor: true, + }); }); it("says no to a passer-by on a repository they can only read", () => { @@ -875,7 +896,13 @@ describe("viewer permission decoding", () => { }), ), ), - ).toEqual({ canWrite: false, canTriage: false, canUpdate: false, didAuthor: false }); + ).toEqual({ + mergeCapabilities: { merge: true, squash: false, rebase: true }, + canWrite: false, + canTriage: false, + canUpdate: false, + didAuthor: false, + }); }); it("reads silence as permission, but not as authorship", () => { @@ -883,6 +910,7 @@ describe("viewer permission decoding", () => { // answer grants it and lets the host refuse; authorship is a fact about who wrote the change, // and claiming it for someone who did not is how an author's own rules get handed out. expect(expectSuccess(decodeViewerPermissionsJson(viewerJson({ pullRequest: null })))).toEqual({ + mergeCapabilities: { merge: true, squash: false, rebase: true }, canWrite: false, canTriage: false, canUpdate: true, diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.ts index 890312b6c854..baa09654691b 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.ts @@ -1073,14 +1073,6 @@ export function buildReviewSubmissionJson(input: { }); } -/** - * `viewerPermission` rides along with the merge settings rather than being asked for on its own: - * `gh repo view --json` serves both out of the same GraphQL repository object, so the viewer's - * standing on the repository costs no request of its own. - */ -export const REPOSITORY_ACCESS_JSON_FIELDS = - "mergeCommitAllowed,squashMergeAllowed,rebaseMergeAllowed,viewerPermission"; - export interface GitHubPullRequestListItem { readonly stack?: PullRequestStackMembership; /** The author's node id, kept so a batch can resolve the avatar the listing does not carry. */ @@ -1528,7 +1520,6 @@ const decodeWorkflowRunApprovals = decodeJsonResult(Schema.Array(RawWorkflowRunA const decodePullRequestHeads = decodeJsonResult(Schema.Array(RawPullRequestHeadSchema)); const decodeActivity = decodeJsonResult(RawActivitySchema); const decodeFileEntry = Schema.decodeUnknownExit(RawPullRequestFileSchema); -const decodeRepositoryAccess = decodeJsonResult(RawRepositoryAccessSchema); const decodeReviewThreads = decodeJsonResult(RawReviewThreadsSchema); const decodeReviewThreadComments = decodeJsonResult(RawReviewThreadCommentsSchema); @@ -2091,7 +2082,7 @@ export function decodeReviewThreadCommentsJson(raw: string): Result.Result< }); } -/** What one `gh repo view` answers: what the repository allows, and where the viewer stands. */ +/** Repository settings returned alongside the pull request viewer permissions. */ export interface GitHubRepositoryAccess { readonly mergeCapabilities: PullRequestMergeCapabilities; readonly canWrite: boolean; @@ -2123,22 +2114,6 @@ function toCanTriage(viewerPermission: string | null | undefined): boolean { return viewerPermission?.trim().toUpperCase() === "TRIAGE" || toCanWrite(viewerPermission); } -export function decodeRepositoryAccessJson( - raw: string, -): Result.Result { - const decoded = decodeRepositoryAccess(raw); - return Result.isSuccess(decoded) - ? Result.succeed({ - mergeCapabilities: { - merge: decoded.success.mergeCommitAllowed, - squash: decoded.success.squashMergeAllowed, - rebase: decoded.success.rebaseMergeAllowed, - }, - canWrite: toCanWrite(decoded.success.viewerPermission), - }) - : Result.fail(decoded.failure); -} - /** * Who a review may be asked of, and who it has already been asked of, in one read. * @@ -2467,15 +2442,10 @@ export interface GitHubViewerAccess { readonly canUpdateBranch?: boolean; } -/** - * The viewer's standing, asked on its own. Only the write path needs this: reading a pull request - * already carries the same three fields on calls it was making anyway, and this exists so that a - * merge or a close is decided by what GitHub says now rather than by what the page was told when - * it loaded. - */ +/** Core detail and write checks share one read of permissions and merge settings. */ export const VIEWER_PERMISSIONS_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { - viewerPermission + mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed viewerPermission pullRequest(number: $number) { viewerCanUpdate viewerDidAuthor } } }`; @@ -2483,7 +2453,7 @@ export const VIEWER_PERMISSIONS_GRAPHQL_QUERY = `query($owner: String!, $name: S const RawViewerPermissionsSchema = Schema.Struct({ data: Schema.Struct({ repository: Schema.Struct({ - viewerPermission: Schema.optional(Schema.NullOr(Schema.String)), + ...RawRepositoryAccessSchema.fields, /** Null for a number that names no pull request the viewer can see. */ pullRequest: Schema.NullOr(RawViewerFieldsSchema), }), @@ -2494,13 +2464,18 @@ const decodeViewerPermissions = decodeJsonResult(RawViewerPermissionsSchema); export function decodeViewerPermissionsJson( raw: string, -): Result.Result { +): Result.Result { const decoded = decodeViewerPermissions(raw); if (!Result.isSuccess(decoded)) { return Result.fail(decoded.failure); } const repository = decoded.success.data.repository; return Result.succeed({ + mergeCapabilities: { + merge: repository.mergeCommitAllowed, + squash: repository.squashMergeAllowed, + rebase: repository.rebaseMergeAllowed, + }, canWrite: toCanWrite(repository.viewerPermission), canTriage: toCanTriage(repository.viewerPermission), ...toPullRequestViewerFields(repository.pullRequest), From f4d4be88d724cf386a6365c471cfb2e459267de4 Mon Sep 17 00:00:00 2001 From: Dominic R Date: Fri, 18 Sep 2026 10:14:38 -0400 Subject: [PATCH 8/8] perf(web): reuse optimized PR detail for hover and opening --- .../pullRequest/GitHubPullRequestCli.test.ts | 351 +++++++++++++++--- .../src/pullRequest/GitHubPullRequestCli.ts | 57 ++- .../GitHubPullRequestProvider.test.ts | 126 +++---- .../pullRequest/GitHubPullRequestProvider.ts | 158 ++++---- .../src/pullRequest/gitHubPullRequestJson.ts | 138 +++++++ .../pullRequest/PullRequestLinkPreview.tsx | 8 +- 6 files changed, 622 insertions(+), 216 deletions(-) diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index 9ff6ccc5c6dd..fd5ab04e8a81 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -17,6 +17,36 @@ import { BASE_COMPARISON_GRAPHQL_QUERY } from "./gitHubPullRequestJson.ts"; const encodeJson = Schema.encodeSync(Schema.fromJsonString(Schema.Unknown)); +const coreResponse = (pullRequest: Readonly> = {}) => ({ + data: { + repository: { + mergeCommitAllowed: true, + squashMergeAllowed: false, + rebaseMergeAllowed: true, + viewerPermission: "WRITE", + pullRequest: { + number: 7, + title: "Pull request 7", + url: "https://github.com/acme/web/pull/7", + headRefName: "feature", + baseRefName: "main", + headRefOid: "abc123", + state: "OPEN", + createdAt: "2026-07-01T00:00:00Z", + updatedAt: "2026-07-02T00:00:00Z", + viewerCanUpdate: true, + viewerDidAuthor: false, + viewerCanUpdateBranch: true, + baseRef: { compare: { behindBy: 2 } }, + reviewRequests: { nodes: [] }, + labels: { nodes: [] }, + commits: { nodes: [] }, + ...pullRequest, + }, + }, + }, +}); + const mockedExecute = vi.fn(); const mockedStackMemberships = vi.fn(() => Effect.succeed(output('{"data":{}}')), @@ -1854,19 +1884,20 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - canned gh response. - JSON.stringify({ - number: 7, - title: "Pull request 7", - url: "https://github.com/acme/web/pull/7", - headRefName: "feat/page", - headRefOid: "abc123", - isCrossRepository: false, - headRepositoryOwner: { login: "acme" }, - baseRefName: "main", - createdAt: "2026-07-01T00:00:00Z", - updatedAt: "2026-07-02T00:00:00Z", - }), + encodeJson( + coreResponse({ + number: 7, + title: "Pull request 7", + url: "https://github.com/acme/web/pull/7", + headRefName: "feat/page", + headRefOid: "abc123", + isCrossRepository: false, + headRepositoryOwner: { login: "acme" }, + baseRefName: "main", + createdAt: "2026-07-01T00:00:00Z", + updatedAt: "2026-07-02T00:00:00Z", + }), + ), ), ), ); @@ -1887,19 +1918,20 @@ layer("GitHubPullRequestCli.layer", (it) => { it.effect("finds and approves every workflow waiting on a maintainer", () => Effect.gen(function* () { const detail = output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - canned gh response. - JSON.stringify({ - number: 7, - title: "Pull request 7", - url: "https://github.com/acme/web/pull/7", - headRefName: "feat/page", - headRefOid: "abc123", - isCrossRepository: true, - headRepositoryOwner: { login: "octocat" }, - baseRefName: "main", - createdAt: "2026-07-01T00:00:00Z", - updatedAt: "2026-07-02T00:00:00Z", - }), + encodeJson( + coreResponse({ + number: 7, + title: "Pull request 7", + url: "https://github.com/acme/web/pull/7", + headRefName: "feat/page", + headRefOid: "abc123", + isCrossRepository: true, + headRepositoryOwner: { login: "octocat" }, + baseRefName: "main", + createdAt: "2026-07-01T00:00:00Z", + updatedAt: "2026-07-02T00:00:00Z", + }), + ), ); const heads = output( // @effect-diagnostics-next-line preferSchemaOverJson:off - canned gh response. @@ -2015,7 +2047,7 @@ layer("GitHubPullRequestCli.layer", (it) => { updatedAt: "2026-07-02T00:00:00Z", }; for (const value of [ - detail, + coreResponse(detail), [ { number: 7, @@ -2025,7 +2057,7 @@ layer("GitHubPullRequestCli.layer", (it) => { }, ], [{ databaseId: 10, workflowName: "build", url: "https://example.com/10" }], - { ...detail, headRefOid: "def456" }, + coreResponse({ ...detail, headRefOid: "def456" }), ]) { mockedExecute.mockReturnValueOnce( Effect.succeed( @@ -2144,19 +2176,20 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - canned gh response. - JSON.stringify({ - number: 7, - title: "Pull request 7", - url: "https://github.com/acme/web/pull/7", - headRefName: "feat/page", - headRefOid: "abc123", - isCrossRepository: true, - headRepositoryOwner: null, - baseRefName: "main", - createdAt: "2026-07-01T00:00:00Z", - updatedAt: "2026-07-02T00:00:00Z", - }), + encodeJson( + coreResponse({ + number: 7, + title: "Pull request 7", + url: "https://github.com/acme/web/pull/7", + headRefName: "feat/page", + headRefOid: "abc123", + isCrossRepository: true, + headRepositoryOwner: null, + baseRefName: "main", + createdAt: "2026-07-01T00:00:00Z", + updatedAt: "2026-07-02T00:00:00Z", + }), + ), ), ), ); @@ -3193,19 +3226,20 @@ layer("GitHubPullRequestCli.layer", (it) => { mockedExecute.mockReturnValueOnce( Effect.succeed( output( - // @effect-diagnostics-next-line preferSchemaOverJson:off - JSON.stringify({ - number: 7, - title: "Progressive detail", - url: "https://github.com/acme/web/pull/7", - author: { login: "octocat" }, - headRefName: "feature", - baseRefName: "main", - createdAt: "2026-07-01T00:00:00Z", - updatedAt: "2026-07-02T00:00:00Z", - body: "Core body", - changedFiles: 2, - }), + encodeJson( + coreResponse({ + number: 7, + title: "Progressive detail", + url: "https://github.com/acme/web/pull/7", + author: { login: "octocat" }, + headRefName: "feature", + baseRefName: "main", + createdAt: "2026-07-01T00:00:00Z", + updatedAt: "2026-07-02T00:00:00Z", + body: "Core body", + changedFiles: 2, + }), + ), ), ), ); @@ -3235,13 +3269,216 @@ layer("GitHubPullRequestCli.layer", (it) => { expect(detail.body).toBe("Core body"); expect(activity.author?.login).toBe("octocat"); - expect(callAt(0).args.at(-1)).toBe( - "number,title,url,author,headRefName,baseRefName,state,isDraft,mergeable,reviewDecision,additions,deletions,createdAt,updatedAt,mergedAt,reviewRequests,labels,statusCheckRollup,body,changedFiles,closedAt,isCrossRepository,headRepositoryOwner,headRefOid,autoMergeRequest", - ); + expect(callAt(0).args).toContain("headRef=refs/pull/7/head"); + expect(callAt(0).args.at(-1)).toContain("viewerCanUpdateBranch"); + expect(detail.viewerAccess.mergeCapabilities).toEqual({ + merge: true, + squash: false, + rebase: true, + }); + expect(detail.comparison).toEqual({ behindBy: 2, viewerCanUpdate: true }); expect(callAt(1).args.at(-1)).toBe("author,comments,reviews,commits"); }), ); + it.effect("decodes reviewers, labels and workflow checks without another detail read", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + encodeJson( + coreResponse({ + baseRef: null, + reviewRequests: { + nodes: [ + { requestedReviewer: { login: "reviewer" } }, + { requestedReviewer: { slug: "maintainers", name: "Maintainers" } }, + ], + }, + labels: { nodes: [{ name: "bug", color: "ff0000" }] }, + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + contexts: { + nodes: [ + { + __typename: "CheckRun", + name: "build", + status: "COMPLETED", + conclusion: "SUCCESS", + checkSuite: { workflowRun: { workflow: { name: "linux" } } }, + }, + { + __typename: "CheckRun", + name: "build", + status: "COMPLETED", + conclusion: "FAILURE", + checkSuite: { workflowRun: { workflow: { name: "windows" } } }, + }, + ], + pageInfo: { hasNextPage: false }, + }, + }, + }, + }, + ], + }, + }), + ), + ), + ), + ); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + const detail = yield* cli.getPullRequestDetail({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }); + expect(mockedExecute).toHaveBeenCalledOnce(); + expect(detail.comparison).toBeNull(); + expect(detail.reviewRequestLogins).toEqual(["reviewer"]); + expect(detail.hasTeamReviewRequest).toBe(true); + expect(detail.labels).toEqual([{ name: "bug", color: "ff0000" }]); + expect(detail.checks).toHaveLength(2); + expect(detail.checksState).toBe("failing"); + }), + ); + + it.effect("reads every check when the combined response has another page", () => + Effect.gen(function* () { + const response = coreResponse({ + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + contexts: { + nodes: [{ name: "first", status: "COMPLETED", conclusion: "SUCCESS" }], + pageInfo: { hasNextPage: true }, + }, + }, + }, + }, + ], + }, + }); + mockedExecute.mockReturnValueOnce(Effect.succeed(output(encodeJson(response)))); + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + encodeJson({ + ...response.data.repository.pullRequest, + reviewRequests: [], + labels: [], + statusCheckRollup: [ + { name: "first", status: "COMPLETED", conclusion: "SUCCESS" }, + { name: "last", status: "COMPLETED", conclusion: "FAILURE" }, + ], + }), + ), + ), + ); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + const detail = yield* cli.getPullRequestDetail({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }); + expect(detail.checks).toHaveLength(2); + expect(detail.checksState).toBe("failing"); + expect(detail.checksTruncated).toBe(false); + expect(callAt(1).args.slice(0, 2)).toEqual(["pr", "view"]); + }), + ); + + it.effect("refuses to combine checks from different head revisions", () => + Effect.gen(function* () { + const response = coreResponse({ + commits: { + nodes: [ + { + commit: { + statusCheckRollup: { + contexts: { + nodes: [], + pageInfo: { hasNextPage: true }, + }, + }, + }, + }, + ], + }, + }); + mockedExecute.mockReturnValueOnce(Effect.succeed(output(encodeJson(response)))); + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + encodeJson({ + ...response.data.repository.pullRequest, + headRefOid: "new-head", + reviewRequests: [], + labels: [], + statusCheckRollup: [], + }), + ), + ), + ); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + const error = yield* Effect.flip( + cli.getPullRequestDetail({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }), + ); + expect(error._tag).toBe("GitHubPullRequestReadError"); + }), + ); + + it.effect("preserves the reserve for automatic detail reads and allows manual checks", () => + Effect.gen(function* () { + const response = coreResponse(); + mockedExecute.mockReturnValue( + Effect.succeed( + output( + encodeJson({ + ...response, + data: { + ...response.data, + rateLimit: { + cost: 1, + limit: 5000, + remaining: 500, + resetAt: "2099-08-13T14:00:00Z", + }, + }, + }), + ), + ), + ); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + const input = { + cwd: "/w", + repository: "acme/web", + host: "github.core-reserve.test", + number: 7, + }; + yield* cli.getPullRequestDetail(input); + const error = yield* Effect.flip(cli.getPullRequestDetail(input)); + expect(error._tag).toBe("SourceControlRateLimitPausedError"); + expect(mockedExecute).toHaveBeenCalledOnce(); + yield* cli + .getPullRequestDetail(input) + .pipe(Effect.provideService(GitHubCli.AllowGitHubReserve, true)); + expect(mockedExecute).toHaveBeenCalledTimes(2); + }), + ); + it.effect("fails a files page too large to read rather than calling the diff whole", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce(Effect.fail(diffRefused)); diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index 517fdfc7468c..723fcfc4d9b9 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -45,6 +45,9 @@ import { decodeActorAvatarsJson, decodePullRequestActivityJson, decodePullRequestDetailJson, + decodePullRequestCoreJson, + PULL_REQUEST_CORE_GRAPHQL_QUERY, + type GitHubPullRequestCore, decodePullRequestPreviewJson, PULL_REQUEST_PREVIEW_GRAPHQL_QUERY, decodePullRequestFilesJson, @@ -95,7 +98,6 @@ import { decodeViewerPermissionsJson, decodeWorkflowRunApprovalsJson, type GitHubBaseComparison, - type GitHubPullRequestDetail, type GitHubPullRequestActivity, type GitHubPullRequestHead, type GitHubPullRequestListItem, @@ -520,7 +522,7 @@ export class GitHubPullRequestCli extends Context.Service< readonly repository: string; readonly host: string; readonly number: number; - }) => Effect.Effect; + }) => Effect.Effect; readonly getPullRequestPreview: (input: { readonly cwd: string; @@ -1518,7 +1520,9 @@ export const make = Effect.gen(function* () { return { oldContents, newContents }; }); - const getPullRequestDetail: GitHubPullRequestCli["Service"]["getPullRequestDetail"] = (input) => + const readLegacyDetail = ( + input: Parameters[0], + ) => github .execute({ cwd: input.cwd, @@ -1547,6 +1551,53 @@ export const make = Effect.gen(function* () { }), ); + const getPullRequestDetail: GitHubPullRequestCli["Service"]["getPullRequestDetail"] = (input) => { + const { owner, name } = parseRepositorySelector(input.repository); + return GitHubCli.AllowGitHubReserve.pipe( + Effect.flatMap((allowReserve) => + graphqlRead({ + allowReserve, + cwd: input.cwd, + host: input.host, + operation: "getPullRequestDetail", + variables: [ + ["-f", `owner=${owner}`], + ["-f", `name=${name}`], + ["-F", `number=${input.number}`], + ["-f", `headRef=refs/pull/${input.number}/head`], + ], + query: PULL_REQUEST_CORE_GRAPHQL_QUERY, + decode: decodePullRequestCoreJson, + }), + ), + ).pipe( + Effect.flatMap((core) => { + if (!core.checksTruncated) return Effect.succeed(core); + // gh already pages check contexts. Keep its complete, deduplicated result for + // large check suites instead of letting the first 100 checks imply success. + return readLegacyDetail(input).pipe( + Effect.flatMap((detail) => + detail.headSha !== core.headSha + ? Effect.fail( + new GitHubPullRequestReadError({ + command: "gh", + cwd: input.cwd, + operation: "getPullRequestDetail", + cause: new Error("Pull request head changed while reading checks."), + }), + ) + : Effect.succeed({ + ...core, + checks: detail.checks, + checksState: detail.checksState, + checksTruncated: false, + }), + ), + ); + }), + ); + }; + const workflowApprovalLimit = 1_000; const workflowApprovalProbeLimit = String(workflowApprovalLimit + 1); const workflowApprovalReadError = (cwd: string, cause: unknown) => diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index b666bb8bc3ab..1882b692f2b0 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -1,5 +1,4 @@ import { describe, expect, it } from "@effect/vitest"; -import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import * as Redacted from "effect/Redacted"; @@ -10,6 +9,18 @@ import * as GitHubPullRequestCli from "./GitHubPullRequestCli.ts"; import { gitHubViewerPermissions, loginAvatarUrl, make } from "./GitHubPullRequestProvider.ts"; import type { GitHubReviewThreadComments } from "./gitHubPullRequestJson.ts"; +const coreFields = { + checksTruncated: false, + comparison: { behindBy: 0, viewerCanUpdate: true }, + viewerAccess: { + canWrite: true, + canTriage: true, + canUpdate: true, + didAuthor: false, + mergeCapabilities: { merge: true, squash: true, rebase: true }, + }, +}; + it.effect("maps credential verification failures without relabeling operation failures", () => Effect.gen(function* () { let verificationFails = true; @@ -230,7 +241,7 @@ describe("gitHubViewerPermissions", () => { }); }); - it.effect("uses the small viewer-access read for core permissions", () => + it.effect("uses the permissions carried by the core read without a second request", () => Effect.gen(function* () { const provider = yield* make; const detail = yield* provider.getChangeRequest({ @@ -277,6 +288,9 @@ describe("gitHubViewerPermissions", () => { Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ getPullRequestDetail: () => Effect.succeed({ + ...coreFields, + comparison: null, + viewerAccess: { ...coreFields.viewerAccess, canWrite: false, canTriage: false }, authorId: null, number: 7, title: "Pull request 7", @@ -286,9 +300,9 @@ describe("gitHubViewerPermissions", () => { headRepositoryOwner: null, headBranch: "feat/page", baseBranch: "main", - state: "open", + state: "open" as const, isDraft: false, - mergeability: "mergeable", + mergeability: "mergeable" as const, reviewDecision: null, additions: 1, deletions: 1, @@ -305,20 +319,21 @@ describe("gitHubViewerPermissions", () => { checks: [], comments: [], commits: [], - }), - getViewerAccess: () => - GitHubCli.PinnedGitHubCredential.pipe( - Effect.map((credential) => ({ - canWrite: false, - canTriage: false, - canUpdate: true, - didAuthor: false, - mergeCapabilities: { - merge: true, - squash: credential?.credentialFingerprint !== "restricted", - rebase: true, - }, - })), + }).pipe( + Effect.flatMap((detail) => + GitHubCli.PinnedGitHubCredential.pipe( + Effect.map((credential) => ({ + ...detail, + viewerAccess: { + ...detail.viewerAccess, + mergeCapabilities: { + ...detail.viewerAccess.mergeCapabilities, + squash: credential?.credentialFingerprint !== "restricted", + }, + }, + })), + ), + ), ), }), ), @@ -361,6 +376,7 @@ describe("gitHubViewerPermissions", () => { Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ getPullRequestDetail: () => Effect.succeed({ + ...coreFields, authorId: null, number: 7, title: "Pull request 7", @@ -424,6 +440,7 @@ describe("gitHubViewerPermissions", () => { }); const openDetail = { + ...coreFields, authorId: null, number: 7, title: "Pull request 7", @@ -455,52 +472,37 @@ const openDetail = { commits: [], }; -it.effect("reads branch comparison and workflow approvals concurrently", () => - Effect.gen(function* () { - const comparisonStarted = yield* Deferred.make(); - const approvalsStarted = yield* Deferred.make(); - const provider = yield* make.pipe( - Effect.provide( - Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ - getPullRequestDetail: () => Effect.succeed(openDetail), - getPullRequestBaseComparison: () => - Deferred.succeed(comparisonStarted, undefined).pipe( - Effect.andThen(Deferred.await(approvalsStarted)), - Effect.as({ behindBy: 2, viewerCanUpdate: true }), - ), - listWorkflowRunsRequiringApproval: () => - Deferred.succeed(approvalsStarted, undefined).pipe( - Effect.andThen(Deferred.await(comparisonStarted)), - Effect.as([{ id: 123, name: "tests", url: "https://example.com/runs/123" }]), - ), - getViewerAccess: () => - Effect.succeed({ - canWrite: true, - canTriage: true, - canUpdate: true, - didAuthor: false, - mergeCapabilities: { merge: true, squash: true, rebase: true }, - }), - }), - ), - ); +it.effect( + "uses the core comparison and permissions while preserving workflow approval checks", + () => + Effect.gen(function* () { + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + getPullRequestDetail: () => + Effect.succeed({ ...openDetail, comparison: { behindBy: 2, viewerCanUpdate: true } }), + listWorkflowRunsRequiringApproval: () => + Effect.succeed([{ id: 123, name: "tests", url: "https://example.com/runs/123" }]), + }), + ), + ); - const detail = yield* provider.getChangeRequest({ - cwd: "/w", - repository: "acme/web", - host: "github.com", - number: 7, - }); + const detail = yield* provider.getChangeRequest({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }); - expect(detail.behindBy).toBe(2); - expect(detail.workflowApprovalsRequired).toBe(1); - expect(detail.checks).toContainEqual({ - name: "tests", - status: "action-required", - description: "A maintainer must approve this workflow before it can run.", - url: "https://example.com/runs/123", - }); - }), + expect(detail.behindBy).toBe(2); + expect(detail.workflowApprovalsRequired).toBe(1); + expect(detail.checks).toContainEqual({ + name: "tests", + status: "action-required", + description: "A maintainer must approve this workflow before it can run.", + url: "https://example.com/runs/123", + }); + }), ); it.effect("does not classify same-repository gates as fork workflow approvals", () => diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index 2c146dd90161..0b76fdda88fc 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -311,98 +311,76 @@ export const make = Effect.gen(function* () { cli.getPullRequestPreview(input).pipe(Effect.mapError(fail("getChangeRequestPreview"))), getChangeRequest: (input) => - Effect.all( - [ - cli.getPullRequestDetail(input).pipe( - Effect.flatMap((pullRequest) => - Effect.all( - { - // Only an open pull request can be behind anything worth saying so about, and - // only one whose head repository is known can be compared at all. - comparison: - pullRequest.state !== "open" || pullRequest.headRepositoryOwner === null - ? Effect.succeed(null) - : cli - .getPullRequestBaseComparison({ - ...input, - headRef: `${pullRequest.headRepositoryOwner}:${pullRequest.headBranch}`, - }) - .pipe(Effect.orElseSucceed(() => null)), - // GitHub omits a fork workflow that has not been approved from the normal check - // rollup. Read the action-required runs by head revision so "all passed" cannot - // be shown while a whole workflow is still waiting to start. - workflowApprovals: - pullRequest.state !== "open" || pullRequest.isCrossRepository !== true - ? Effect.succeed({ - runs: [] as ReadonlyArray, - unavailable: false, - }) - : pullRequest.headSha == null || pullRequest.headRepositoryOwner == null - ? Effect.succeed({ - runs: [] as ReadonlyArray, - unavailable: true, - }) - : cli - .listWorkflowRunsRequiringApproval({ - ...input, - headSha: pullRequest.headSha, - headBranch: pullRequest.headBranch, - headRepositoryOwner: pullRequest.headRepositoryOwner, - isCrossRepository: true, - }) - .pipe( - Effect.matchEffect({ - onFailure: (error) => - error._tag === "GitHubCliRateLimitError" || - error._tag === "SourceControlRateLimitPausedError" - ? Effect.fail(error) - : Effect.succeed({ - runs: [] as ReadonlyArray, - unavailable: true, - }), - onSuccess: (runs) => Effect.succeed({ runs, unavailable: false }), + cli.getPullRequestDetail(input).pipe( + Effect.flatMap((pullRequest) => { + // Fork workflows awaiting approval are absent from the normal check rollup. + const approvals = + pullRequest.state !== "open" || pullRequest.isCrossRepository !== true + ? Effect.succeed({ + runs: [] as ReadonlyArray, + unavailable: false, + }) + : pullRequest.headSha == null || pullRequest.headRepositoryOwner == null + ? Effect.succeed({ + runs: [] as ReadonlyArray, + unavailable: true, + }) + : cli + .listWorkflowRunsRequiringApproval({ + ...input, + headSha: pullRequest.headSha, + headBranch: pullRequest.headBranch, + headRepositoryOwner: pullRequest.headRepositoryOwner, + isCrossRepository: true, + }) + .pipe( + Effect.matchEffect({ + onFailure: (error) => + error._tag === "GitHubCliRateLimitError" || + error._tag === "SourceControlRateLimitPausedError" + ? Effect.fail(error) + : Effect.succeed({ + runs: [] as ReadonlyArray, + unavailable: true, }), - ), - }, - { concurrency: 2 }, - ).pipe(Effect.map((extra) => ({ pullRequest, ...extra }))), - ), - ), - // Repository merge settings travel with the viewer permissions. - cli.getViewerAccess(input), - ], - { concurrency: 2 }, - ).pipe( + onSuccess: (runs) => Effect.succeed({ runs, unavailable: false }), + }), + ); + return approvals.pipe( + Effect.map((workflowApprovals): ProviderChangeRequestDetail => ({ + ...pullRequest, + author: withAvatar(pullRequest.author, new Map(), input.host), + checks: withWorkflowApprovals( + pullRequest.checks, + workflowApprovals.runs, + workflowApprovals.unavailable, + ), + ...(workflowApprovals.unavailable + ? {} + : { workflowApprovalsRequired: workflowApprovals.runs.length }), + reviewers: pullRequest.reviewRequestLogins.map((login) => ({ + login, + name: null, + avatarUrl: null, + })), + mergeCapabilities: pullRequest.viewerAccess.mergeCapabilities, + viewerPermissions: gitHubViewerPermissions({ + ...pullRequest.viewerAccess, + canUpdateBranch: pullRequest.comparison?.viewerCanUpdate === true, + }), + baseComparison: + pullRequest.comparison === null || pullRequest.comparison.behindBy === null + ? "unknown" + : pullRequest.comparison.behindBy > 0 + ? "behind" + : "up-to-date", + ...(pullRequest.comparison?.behindBy == null + ? {} + : { behindBy: pullRequest.comparison.behindBy }), + })), + ); + }), Effect.mapError(fail("getChangeRequest")), - Effect.map(([detail, viewerAccess]): ProviderChangeRequestDetail => ({ - ...detail.pullRequest, - author: withAvatar(detail.pullRequest.author, new Map(), input.host), - checks: withWorkflowApprovals( - detail.pullRequest.checks, - detail.workflowApprovals.runs, - detail.workflowApprovals.unavailable, - ), - ...(detail.workflowApprovals.unavailable - ? {} - : { workflowApprovalsRequired: detail.workflowApprovals.runs.length }), - reviewers: detail.pullRequest.reviewRequestLogins.map((login) => ({ - login, - name: null, - avatarUrl: null, - })), - mergeCapabilities: viewerAccess.mergeCapabilities, - viewerPermissions: gitHubViewerPermissions({ - ...viewerAccess, - canUpdateBranch: detail.comparison?.viewerCanUpdate === true, - }), - baseComparison: - detail.comparison === null || detail.comparison.behindBy === null - ? "unknown" - : detail.comparison.behindBy > 0 - ? "behind" - : "up-to-date", - ...(detail.comparison?.behindBy == null ? {} : { behindBy: detail.comparison.behindBy }), - })), ), getChangeRequestActivity: (input) => diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.ts index baa09654691b..a011702d685e 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.ts @@ -597,6 +597,62 @@ const RawRepositoryAccessSchema = Schema.Struct({ viewerPermission: Schema.optional(Schema.NullOr(Schema.String)), }); +const RawCoreSchema = Schema.Struct({ + data: Schema.Struct({ + repository: Schema.Struct({ + ...RawRepositoryAccessSchema.fields, + pullRequest: Schema.Struct({ + ...RawDetailSchema.fields, + ...RawViewerFieldsSchema.fields, + viewerCanUpdateBranch: Schema.Boolean, + baseRef: Schema.NullOr( + Schema.Struct({ + compare: Schema.NullOr(Schema.Struct({ behindBy: Schema.Int })), + }), + ), + reviewRequests: Schema.Struct({ + nodes: Schema.Array( + Schema.Struct({ requestedReviewer: Schema.NullOr(RawReviewRequestSchema) }), + ), + }), + labels: Schema.Struct({ nodes: Schema.Array(RawLabelSchema) }), + commits: Schema.Struct({ + nodes: Schema.Array( + Schema.Struct({ + commit: Schema.Struct({ + statusCheckRollup: Schema.NullOr( + Schema.Struct({ + contexts: Schema.Struct({ + nodes: Schema.Array( + Schema.Struct({ + ...RawCheckSchema.fields, + checkSuite: Schema.optional( + Schema.NullOr( + Schema.Struct({ + workflowRun: Schema.NullOr( + Schema.Struct({ + workflow: Schema.NullOr(Schema.Struct({ name: Schema.String })), + }), + ), + }), + ), + ), + }), + ), + pageInfo: Schema.Struct({ hasNextPage: Schema.Boolean }), + }), + }), + ), + }), + }), + ), + }), + }), + }), + }), +}); +const decodeCore = decodeJsonResult(RawCoreSchema); + const RawPullRequestFileSchema = Schema.Struct({ filename: Schema.String, status: Schema.optional(Schema.NullOr(Schema.String)), @@ -646,6 +702,40 @@ export const PULL_REQUEST_LIST_JSON_FIELDS = export const PULL_REQUEST_DETAIL_JSON_FIELDS = `${PULL_REQUEST_LIST_JSON_FIELDS},body,changedFiles,closedAt,isCrossRepository,headRepositoryOwner,headRefOid,autoMergeRequest`; +/** Pull refs let the comparison share the detail read without first resolving a fork branch. */ +export const PULL_REQUEST_CORE_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!, $headRef: String!) { + repository(owner: $owner, name: $name) { + mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed viewerPermission + pullRequest(number: $number) { + number title url body state isDraft mergeable reviewDecision + additions deletions changedFiles createdAt updatedAt mergedAt closedAt + headRefName baseRefName headRefOid isCrossRepository + headRepositoryOwner { login } + author { login avatarUrl ... on User { id name } } + autoMergeRequest { mergeMethod } + viewerCanUpdate viewerDidAuthor viewerCanUpdateBranch + baseRef { compare(headRef: $headRef) { behindBy } } + reviewRequests(first: 100) { + nodes { requestedReviewer { ... on User { login name } ... on Bot { login } ... on Team { slug name } } } + } + labels(first: 100) { nodes { name color } } + commits(last: 1) { + nodes { commit { statusCheckRollup { contexts(first: 100) { + nodes { + __typename + ... on StatusContext { context state targetUrl createdAt description } + ... on CheckRun { + name status conclusion startedAt completedAt detailsUrl + checkSuite { workflowRun { workflow { name } } } + } + } + pageInfo { hasNextPage } + } } } } + } + } + } +}`; + export const PULL_REQUEST_PREVIEW_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { @@ -1719,6 +1809,54 @@ export function decodePullRequestStatsJson( return Result.succeed(stats); } +export interface GitHubPullRequestCore extends GitHubPullRequestDetail { + readonly viewerAccess: GitHubViewerAccess & GitHubRepositoryAccess; + readonly comparison: GitHubBaseComparison | null; + readonly checksTruncated: boolean; +} + +export function decodePullRequestCoreJson( + raw: string, +): Result.Result { + const decoded = decodeCore(raw); + if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); + const repository = decoded.success.data.repository; + const pr = repository.pullRequest; + const contexts = pr.commits.nodes[0]?.commit.statusCheckRollup?.contexts; + return Result.succeed({ + ...toDetail({ + ...pr, + reviewRequests: pr.reviewRequests.nodes.flatMap(({ requestedReviewer }) => + requestedReviewer === null ? [] : [requestedReviewer], + ), + labels: pr.labels.nodes, + statusCheckRollup: + contexts?.nodes.map((check) => ({ + ...check, + workflowName: check.checkSuite?.workflowRun?.workflow?.name ?? null, + })) ?? [], + }), + viewerAccess: { + canWrite: toCanWrite(repository.viewerPermission), + canTriage: toCanTriage(repository.viewerPermission), + ...toPullRequestViewerFields(pr), + mergeCapabilities: { + merge: repository.mergeCommitAllowed, + squash: repository.squashMergeAllowed, + rebase: repository.rebaseMergeAllowed, + }, + }, + comparison: + pr.state !== "OPEN" || pr.baseRef?.compare == null + ? null + : { + behindBy: pr.baseRef.compare.behindBy, + viewerCanUpdate: pr.viewerCanUpdateBranch, + }, + checksTruncated: contexts?.pageInfo.hasNextPage === true, + }); +} + export function decodePullRequestDetailJson( raw: string, ): Result.Result { diff --git a/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx b/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx index fd7a508a519f..bbdbd9e16f99 100644 --- a/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx +++ b/apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx @@ -34,9 +34,9 @@ export function PullRequestLinkPreview({ }) { const [open, setOpen] = useState(false); const [resolvingClick, setResolvingClick] = useState(false); - const previewQuery = useEnvironmentQuery( + const detailQuery = useEnvironmentQuery( open - ? pullRequestEnvironment.preview({ + ? pullRequestEnvironment.detail({ environmentId: target.environmentId, input: target.input, }) @@ -68,7 +68,7 @@ export function PullRequestLinkPreview({ }, }) : link; - const detail = previewQuery.data; + const detail = detailQuery.data; const state = detail === null ? null @@ -83,7 +83,7 @@ export function PullRequestLinkPreview({ return ( - {detail !== null || previewQuery.error !== null ? ( + {detail !== null || detailQuery.error !== null ? ( {detail === null ? (