diff --git a/apps/server/src/auth/RpcAuthorization.test.ts b/apps/server/src/auth/RpcAuthorization.test.ts index a730d7c97ab2..defee4f7d536 100644 --- a/apps/server/src/auth/RpcAuthorization.test.ts +++ b/apps/server/src/auth/RpcAuthorization.test.ts @@ -79,6 +79,9 @@ describe("RPC authorization scopes", () => { it("reads the reviewer menu under the same scope as the pull request it belongs to", () => { // The candidate list is a read like the detail beside it, and asking somebody for a review is // a write like every other pull request operation. + expect(requiredScopeForRpcMethod(WS_METHODS.pullRequestsChecks)).toBe( + AuthOrchestrationReadScope, + ); expect(requiredScopeForRpcMethod(WS_METHODS.pullRequestsReviewerCandidates)).toBe( requiredScopeForRpcMethod(WS_METHODS.pullRequestsDetail), ); diff --git a/apps/server/src/auth/RpcAuthorization.ts b/apps/server/src/auth/RpcAuthorization.ts index 05ad13974edd..ea0c4c3f02e5 100644 --- a/apps/server/src/auth/RpcAuthorization.ts +++ b/apps/server/src/auth/RpcAuthorization.ts @@ -94,6 +94,7 @@ export const RPC_REQUIRED_SCOPES = { [WS_METHODS.pullRequestsStack]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsLinkedThreads]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsDetail]: AuthOrchestrationReadScope, + [WS_METHODS.pullRequestsChecks]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsActivity]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsThreadComments]: AuthOrchestrationReadScope, [WS_METHODS.pullRequestsDiffFileContents]: AuthOrchestrationReadScope, diff --git a/apps/server/src/environment/ServerEnvironment.ts b/apps/server/src/environment/ServerEnvironment.ts index d9445cfe29c2..5ba7edffd0af 100644 --- a/apps/server/src/environment/ServerEnvironment.ts +++ b/apps/server/src/environment/ServerEnvironment.ts @@ -221,6 +221,7 @@ export const make = Effect.gen(function* () { questionAttachments: true, fileAttachments: { maxUploadBytes: PROVIDER_SEND_TURN_MAX_FILE_BYTES }, pullRequests: true, + pullRequestChecks: true, inlineMessageContext: true, threadSettlement: true, threadAutoSettlement: true, diff --git a/apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts b/apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts index db06a360a55c..7c2d58af950e 100644 --- a/apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts @@ -1,11 +1,134 @@ -import { describe, expect, it } from "vite-plus/test"; +import { describe, expect, it } from "@effect/vitest"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; +import * as Result from "effect/Result"; import * as BitbucketApi from "../sourceControl/BitbucketApi.ts"; +import * as BitbucketPullRequestApi from "./BitbucketPullRequestApi.ts"; +import { decodePullRequestJson } from "./bitbucketPullRequestJson.ts"; import { bitbucketProviderFailure, bitbucketViewerPermissions, + make, } from "./BitbucketPullRequestProvider.ts"; +for (const operation of [ + "getMergeability", + "listChecks", + "getRepositoryPermission", + "listComments", + "listCommits", +] as const) { + it.effect.each(["response", "body read"])( + `preserves rate limits from ${operation} on %s errors while recovering other optional-read failures`, + (variant) => + Effect.gen(function* () { + const pullRequest = Result.getOrThrow( + decodePullRequestJson(`{ + "id": 1, "title": "Check polling", "state": "OPEN", + "source": { "branch": { "name": "feature" } }, + "destination": { "branch": { "name": "main" } }, + "created_on": "2026-09-16T00:00:00Z", + "updated_on": "2026-09-16T00:00:00Z", + "links": { "html": { "href": "https://bitbucket.org/acme/web/pull-requests/1" } } + }`), + ); + for (const status of [429, 403]) { + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(BitbucketPullRequestApi.BitbucketPullRequestApi)({ + getPullRequest: () => Effect.succeed(pullRequest), + getDiffStat: () => Effect.succeed({ additions: 0, deletions: 0, changedFiles: 0 }), + getMergeability: () => Effect.succeed("unknown" as const), + listChecks: () => Effect.succeed([]), + getRepositoryPermission: () => Effect.succeed(true), + listComments: () => Effect.succeed({ comments: [], threads: [], truncated: false }), + listCommits: () => Effect.succeed([]), + [operation]: () => + Effect.fail( + variant === "response" + ? new BitbucketApi.BitbucketResponseError({ + operation: "request", + status, + responseBodyLength: 0, + retryAt: 120_000, + }) + : new BitbucketApi.BitbucketResponseBodyReadError({ + operation: "request", + status, + cause: new Error("response stream failed"), + retryAt: 120_000, + }), + ), + }), + ), + ); + const reference = { + cwd: "/repo", + repository: "acme/web", + number: 1, + host: "bitbucket.org", + }; + const result = yield* operation === "listComments" || operation === "listCommits" + ? Effect.result(provider.getChangeRequestActivity(reference)) + : Effect.result(provider.getChangeRequest(reference)); + if (status === 429) { + expect(result).toMatchObject({ + _tag: "Failure", + failure: { reason: "rate-limited", retryAt: 120_000 }, + }); + } else { + expect(result._tag).toBe("Success"); + } + } + }), + ); +} + +it.effect("reads checks and PR state without diff, mergeability, or permission requests", () => + Effect.gen(function* () { + const pullRequest = Result.getOrThrow( + decodePullRequestJson(`{ + "id": 1, "title": "Checks", "state": "OPEN", + "source": { "branch": { "name": "feature" } }, + "destination": { "branch": { "name": "main" } }, + "created_on": "2026-09-16T00:00:00Z", "updated_on": "2026-09-16T00:00:00Z", + "links": { "html": { "href": "https://bitbucket.org/acme/web/pull-requests/1" } } + }`), + ); + let limited = false; + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(BitbucketPullRequestApi.BitbucketPullRequestApi)({ + getPullRequest: () => Effect.succeed(pullRequest), + listChecks: () => + limited + ? Effect.fail( + new BitbucketApi.BitbucketResponseError({ + operation: "request", + status: 429, + responseBodyLength: 0, + retryAt: 120_000, + }), + ) + : Effect.succeed([ + { name: "build", status: "failure" as const, description: null, url: null }, + ]), + }), + ), + ); + const read = provider.getChangeRequestChecks; + if (read === undefined) return yield* Effect.die("checks read missing"); + const input = { cwd: "/repo", repository: "acme/web", host: "bitbucket.org", number: 1 }; + expect((yield* read(input)).checks[0]?.status).toBe("failure"); + limited = true; + expect(yield* Effect.flip(read(input))).toMatchObject({ + reason: "rate-limited", + retryAt: 120_000, + }); + }), +); + describe("bitbucketProviderFailure", () => { it("treats only an HTTP 401 as unusable credentials", () => { const responseError = (status: number) => diff --git a/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts b/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts index 3b5b93d11c46..b096d1f501bd 100644 --- a/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts +++ b/apps/server/src/pullRequest/BitbucketPullRequestProvider.ts @@ -68,7 +68,10 @@ export function bitbucketProviderFailure( if (error._tag === "BitbucketResponseError" && error.status === 401) { return { reason: "unauthenticated" }; } - if (error._tag === "BitbucketResponseError" && error.status === 429) { + if ( + (error._tag === "BitbucketResponseError" || error._tag === "BitbucketResponseBodyReadError") && + error.status === 429 + ) { return { reason: "rate-limited", ...(error.retryAt === undefined ? {} : { retryAt: error.retryAt }), @@ -117,6 +120,31 @@ export const make = Effect.gen(function* () { cause: error, }); + const recoverRead = ( + read: Effect.Effect, + fallback: A, + ) => { + const recover = () => Effect.succeed(fallback); + return Effect.catchTags(read, { + BitbucketResponseError: (error) => (error.status === 429 ? Effect.fail(error) : recover()), + BitbucketUntrustedUrlError: recover, + BitbucketRepositoryLocatorError: recover, + BitbucketRequestError: recover, + BitbucketResponseBodyReadError: (error) => + error.status === 429 ? Effect.fail(error) : recover(), + BitbucketResponseDecodeError: recover, + BitbucketRepositoryVcsResolveError: recover, + BitbucketRepositoryRemotesListError: recover, + BitbucketRepositoryRemoteNotFoundError: recover, + BitbucketPullRequestBodyReadError: recover, + BitbucketCheckoutError: recover, + BitbucketPullRequestReadError: recover, + BitbucketViewerUnavailableError: recover, + BitbucketRepositoryUnsupportedError: recover, + BitbucketDiffCommitError: recover, + }); + }; + const provider: PullRequestProviderApi = { kind: "bitbucket", capabilities: CAPABILITIES, @@ -145,18 +173,24 @@ export const make = Effect.gen(function* () { })), ), + getChangeRequestChecks: (input) => + Effect.all([api.getPullRequest(input), api.listChecks(input)], { concurrency: 2 }).pipe( + Effect.map(([pullRequest, checks]) => ({ state: pullRequest.state, checks })), + Effect.mapError(fail("getChangeRequestChecks")), + ), + getChangeRequest: (input) => { const target = { repository: input.repository, number: input.number }; return Effect.all( [ api.getPullRequest(target), api.getDiffStat(target), - api.getMergeability(target).pipe(Effect.orElseSucceed(() => "unknown" as const)), - api.listChecks(target).pipe(Effect.orElseSucceed(() => [])), + recoverRead(api.getMergeability(target), "unknown" as const), + recoverRead(api.listChecks(target), []), // A permission that could not be read is an unknown one, which is granted: a hidden // Merge leaves someone entitled to it with no way through, and one Bitbucket refuses // at least says why. - api.getRepositoryPermission(target).pipe(Effect.orElseSucceed(() => true)), + recoverRead(api.getRepositoryPermission(target), true), ], { concurrency: 5 }, ).pipe( @@ -195,10 +229,8 @@ export const make = Effect.gen(function* () { // Reviews ride on the pull request itself, so this inexpensive core read is repeated // here rather than making the core response wait for the conversation endpoints. api.getPullRequest(target), - api - .listComments(target) - .pipe(Effect.orElseSucceed(() => ({ comments: [], threads: [], truncated: true }))), - api.listCommits(target).pipe(Effect.orElseSucceed(() => [])), + recoverRead(api.listComments(target), { comments: [], threads: [], truncated: true }), + recoverRead(api.listCommits(target), []), ], { concurrency: 3 }, ).pipe( diff --git a/apps/server/src/pullRequest/ForgejoPullRequestProvider.ts b/apps/server/src/pullRequest/ForgejoPullRequestProvider.ts index 526be38f7df7..0039811838a4 100644 --- a/apps/server/src/pullRequest/ForgejoPullRequestProvider.ts +++ b/apps/server/src/pullRequest/ForgejoPullRequestProvider.ts @@ -233,6 +233,19 @@ export const make = Effect.gen(function* () { }, ), getChangeRequestSummary: (input) => getPull(input).pipe(Effect.map(forgejoChangeRequest)), + getChangeRequestChecks: Effect.fn("ForgejoPullRequestProvider.getChangeRequestChecks")( + function* (input) { + const pr = yield* getPull(input); + const statuses = yield* page( + { + ...input, + path: `${repoPath(input)}/statuses/${encodeURIComponent(pr.head.sha)}?sort=recentupdate`, + }, + ForgejoStatus, + ); + return { state: forgejoChangeRequest(pr).state, checks: forgejoChecks(statuses.items) }; + }, + ), getChangeRequest: Effect.fn("ForgejoPullRequestProvider.getChangeRequest")(function* (input) { const [pr, repo, viewer] = yield* Effect.all( [getPull(input), getRepo(input), getViewer(input)], diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index c2571d91d563..dad7bb51f2f1 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -2,8 +2,10 @@ import { describe, expect, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import * as Redacted from "effect/Redacted"; +import * as Result from "effect/Result"; import type { PullRequestReaction } from "@t3tools/contracts"; +import { decodePullRequestDetailJson } from "./gitHubPullRequestJson.ts"; import * as GitHubCli from "../sourceControl/GitHubCli.ts"; import * as GitHubPullRequestCli from "./GitHubPullRequestCli.ts"; import { gitHubViewerPermissions, loginAvatarUrl, make } from "./GitHubPullRequestProvider.ts"; @@ -47,6 +49,54 @@ it.effect("maps credential verification failures without relabeling operation fa }), ); +it.effect("refreshes checks without permissions or comparison reads", () => + Effect.gen(function* () { + let reads = 0; + const snapshot = Result.getOrThrow( + decodePullRequestDetailJson(`{ + "number": 7, "title": "Checks", "url": "https://github.com/acme/web/pull/7", + "headRefName": "feature", "baseRefName": "main", "state": "OPEN", + "createdAt": "2026-07-01T00:00:00Z", "updatedAt": "2026-07-01T00:00:00Z" + }`), + ); + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ + getPullRequestDetail: () => + Effect.sync(() => { + reads++; + return { + ...snapshot, + state: reads === 3 ? ("merged" as const) : ("open" as const), + checks: [ + { + name: "build", + status: reads === 1 ? ("pending" as const) : ("success" as const), + description: null, + url: null, + }, + ], + }; + }), + }), + ), + ); + const read = provider.getChangeRequestChecks; + if (read === undefined) return yield* Effect.die("checks read missing"); + for (let tick = 1; tick <= 3; tick++) { + const result = yield* read({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }); + expect(result.checks[0]?.status).toBe(tick === 1 ? "pending" : "success"); + expect(result.state).toBe(tick === 3 ? "merged" : "open"); + } + expect(reads).toBe(3); + }), +); + it.effect("uses one narrow read for a linked pull request summary", () => Effect.gen(function* () { let summaryReads = 0; @@ -338,6 +388,11 @@ describe("gitHubViewerPermissions", () => { number: 7, }); + const readChecks = provider.getChangeRequestChecks; + if (readChecks === undefined) return yield* Effect.die("checks read missing"); + expect( + yield* readChecks({ cwd: "/w", repository: "acme/web", host: "github.com", number: 7 }), + ).toEqual({ state: detail.state, checks: detail.checks }); expect(detail.workflowApprovalsRequired).toBe(1); expect(detail.checks).toEqual([ { diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index 1d003a970ee8..b5ef1a135e64 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -18,6 +18,7 @@ import { type ProviderChangeRequestActivity, type ProviderChangeRequestDetail, type PullRequestProviderApi, + type ProviderRepositoryRef, } from "./PullRequestProvider.ts"; import type { GitHubViewerAccess, GitHubWorkflowRunApproval } from "./gitHubPullRequestJson.ts"; @@ -227,6 +228,56 @@ export const make = Effect.gen(function* () { cause: error, }); + const readChecks = (input: ProviderRepositoryRef & { readonly number: number }) => + cli.getPullRequestDetail(input).pipe( + Effect.flatMap((pullRequest) => + (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 }), + }), + ) + ).pipe( + Effect.map((workflowApprovals) => ({ + ...pullRequest, + checks: withWorkflowApprovals( + pullRequest.checks, + workflowApprovals.runs, + workflowApprovals.unavailable, + ), + ...(workflowApprovals.unavailable + ? {} + : { workflowApprovalsRequired: workflowApprovals.runs.length }), + })), + ), + ), + ); + const provider: PullRequestProviderApi = { kind: "github", capabilities: CAPABILITIES, @@ -336,68 +387,29 @@ export const make = Effect.gen(function* () { getChangeRequestStack: (input) => cli.getPullRequestStack(input).pipe(Effect.mapError(fail("getChangeRequestStack"))), + getChangeRequestChecks: (input) => + readChecks(input).pipe( + Effect.map(({ state, checks }) => ({ state, checks })), + Effect.mapError(fail("getChangeRequestChecks")), + ), + getChangeRequest: (input) => Effect.all( [ - cli.getPullRequestDetail(input).pipe( + readChecks(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 }), - }), - ), - }).pipe(Effect.map((extra) => ({ pullRequest, ...extra }))), + (pullRequest.state !== "open" || pullRequest.headRepositoryOwner === null + ? Effect.succeed(null) + : cli + .getPullRequestBaseComparison({ + ...input, + headRef: `${pullRequest.headRepositoryOwner}:${pullRequest.headBranch}`, + }) + .pipe(Effect.orElseSucceed(() => null)) + ).pipe(Effect.map((comparison) => ({ pullRequest, comparison }))), ), ), - 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. + getRepositoryAccess(input), cli.getViewerAccess(input), ], { concurrency: 3 }, @@ -405,14 +417,6 @@ export const make = Effect.gen(function* () { Effect.mapError(fail("getChangeRequest")), Effect.map(([detail, repository, viewerAccess]): ProviderChangeRequestDetail => ({ ...detail.pullRequest, - 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, diff --git a/apps/server/src/pullRequest/GitLabPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitLabPullRequestProvider.test.ts index 5d36d58dfc2b..d506aace9489 100644 --- a/apps/server/src/pullRequest/GitLabPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitLabPullRequestProvider.test.ts @@ -88,6 +88,23 @@ describe("getChangeRequest base freshness", () => { reviewerIds: [], }; + it.effect("reads checks without fetching project settings", () => + Effect.gen(function* () { + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(GitLabPullRequestCli.GitLabPullRequestCli)({ + getMergeRequestDetail: () => Effect.succeed(detail), + }), + ), + ); + const read = provider.getChangeRequestChecks; + if (read === undefined) return yield* Effect.die("checks read missing"); + expect( + yield* read({ cwd: "/w", repository: "group/subgroup/web", host: "gitlab.com", number: 7 }), + ).toEqual({ state: "open", checks: [] }); + }), + ); + const readWith = (divergence: { readonly divergedCommits?: number }) => Effect.gen(function* () { const provider = yield* make; diff --git a/apps/server/src/pullRequest/GitLabPullRequestProvider.ts b/apps/server/src/pullRequest/GitLabPullRequestProvider.ts index 46fce2884279..0574ef333240 100644 --- a/apps/server/src/pullRequest/GitLabPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitLabPullRequestProvider.ts @@ -138,6 +138,12 @@ export const make = Effect.gen(function* () { Effect.map((batch) => ({ ...batch, continues: true })), ), + getChangeRequestChecks: (input) => + cli.getMergeRequestDetail(input).pipe( + Effect.map(({ state, checks }) => ({ state, checks })), + Effect.mapError(fail("getChangeRequestChecks")), + ), + getChangeRequest: (input) => Effect.all( [ diff --git a/apps/server/src/pullRequest/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index 405eb6ae9e6c..b99e3b96da7c 100644 --- a/apps/server/src/pullRequest/PullRequestProvider.ts +++ b/apps/server/src/pullRequest/PullRequestProvider.ts @@ -9,6 +9,7 @@ import type { PullRequestCapabilities, PullRequestChecksState, PullRequestCheck, + PullRequestChecks, PullRequestComment, PullRequestCommit, PullRequestInvolvement, @@ -369,6 +370,10 @@ export interface PullRequestProviderApi { }>; }) => Effect.Effect, PullRequestProviderError>; + readonly getChangeRequestChecks?: ( + input: ProviderRepositoryRef & { readonly number: number }, + ) => Effect.Effect; + readonly getChangeRequest: ( input: ProviderRepositoryRef & { readonly number: number }, ) => Effect.Effect; diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index c576101fa5a9..5bc19fe162aa 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -1,6 +1,7 @@ import * as NodeServices from "@effect/platform-node/NodeServices"; import * as KeyValueStore from "effect/unstable/persistence/KeyValueStore"; import { assert, it } from "@effect/vitest"; +import * as Clock from "effect/Clock"; import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; import * as Fiber from "effect/Fiber"; @@ -1524,6 +1525,88 @@ it.effect("stops new reads after a rate limit while leaving manual actions avail }), ); +for (const [provider, host] of [ + ["github", "github.com"], + ["gitlab", "gitlab.com"], + ["forgejo", "code.example.test"], + ["bitbucket", "bitbucket.org"], + ["azure-devops", "dev.azure.com"], +] as const) { + it.effect.each(["detail", "checks"] as const)( + `shares fresh ${provider} %s reads and respects rate-limit resets`, + (read) => + Effect.gen(function* () { + let calls = 0; + let limited = false; + const repository = provider === "azure-devops" ? "web" : "acme/web"; + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "web", workspaceRoot: "/repo", repository, provider, host }), + ], + providers: [ + fakeProvider(provider, { + [read === "detail" ? "getChangeRequest" : "getChangeRequestChecks"]: () => + Effect.gen(function* () { + calls += 1; + if (limited) { + return yield* new PullRequestProviderError({ + provider, + operation: "getChangeRequest", + reason: "rate-limited", + detail: "Retry after the reset.", + retryAt: (yield* Clock.currentTimeMillis) + 120_000, + }); + } + return hostedChangeRequest("current details"); + }), + }), + ], + }); + const reference = { + projectId: "p1" as ProjectId, + repository, + number: 1, + allowStale: false, + }; + yield* Effect.all([service[read](reference), service[read](reference)], { concurrency: 2 }); + assert.strictEqual(calls, 1); + yield* TestClock.adjust("45 seconds"); + limited = true; + yield* Effect.flip(service[read](reference)); + assert.strictEqual(calls, 2); + yield* TestClock.adjust("45 seconds"); + yield* Effect.flip(service[read](reference)); + assert.strictEqual(calls, 2); + yield* TestClock.adjust("75 seconds"); + limited = false; + const recovered = yield* service[read](reference); + assert.strictEqual(recovered?.state, "open"); + assert.strictEqual(calls, 3); + yield* service.invalidate({ reference }); + yield* service[read](reference); + assert.strictEqual(calls, 4); + }), + ); +} + +it.effect("does not fall back to full detail when checks are unsupported", () => + Effect.gen(function* () { + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "web", workspaceRoot: "/repo", repository: "acme/web" }), + ], + providers: [ + fakeProvider("github", { + getChangeRequest: () => Effect.die("Unexpected full detail read"), + }), + ], + }); + assert.isNull( + yield* service.checks({ projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }), + ); + }), +); + it.effect("uses a manual rate limit to pause later reads", () => Effect.gen(function* () { let listCalls = 0; @@ -3869,7 +3952,7 @@ it.effect("shares linked summaries and reuses them for display without asking th it.effect("keeps routed reads separate when the GitHub account changes", () => Effect.gen(function* () { - for (const operation of ["summary", "detail", "diff"] as const) { + for (const operation of ["summary", "detail", "checks", "diff"] as const) { let failing = false; let calls = 0; const read = () => @@ -3886,6 +3969,7 @@ it.effect("keeps routed reads separate when the GitHub account changes", () => providers: [ fakeProvider("github", { getChangeRequestSummary: read, + getChangeRequestChecks: read, getChangeRequest: read, getDiff: () => read().pipe( @@ -3915,7 +3999,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", "checks", "diff"] as const) { let credential = "broad"; let calls = 0; const read = () => @@ -3940,6 +4024,7 @@ it.effect("isolates routed caches for two credentials belonging to the same acco }), ), getChangeRequest: read, + getChangeRequestChecks: read, getChangeRequestSummary: read, getDiff: () => read().pipe( diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index d0d57f4ae8a6..b8aeacfe6a33 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 PullRequestChecks, 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 checks: ( + 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.getChangeRequestChecks === undefined + ? {} + : { getChangeRequestChecks: wrap("getChangeRequestChecks", api.getChangeRequestChecks) }), ...(api.getChangeRequestSummary === undefined ? {} : { @@ -2662,6 +2669,30 @@ export const make = Effect.gen(function* () { return Cache.get(listCache, key); }; + const checksCache = yield* Cache.makeWith( + (key: string) => { + const input = refOfCacheKey(key); + return requireProject(input).pipe( + Effect.flatMap((project) => + project.api.getChangeRequestChecks === undefined + ? Effect.succeed(null) + : project.api + .getChangeRequestChecks({ + cwd: project.project.workspaceRoot, + repository: project.repository, + host: project.host, + number: input.number, + }) + .pipe(Effect.mapError(toPullRequestError("checks"))), + ), + ); + }, + { + capacity: DETAIL_CACHE_CAPACITY, + timeToLive: (exit) => (Exit.isSuccess(exit) ? DETAIL_CACHE_TTL : Duration.zero), + }, + ); + const detailCache = yield* Cache.makeWith( (key: string) => { const statsKey = statsCacheKey(key); @@ -2959,6 +2990,7 @@ export const make = Effect.gen(function* () { ), refreshAfterTurn, detail: credentialCached(detail), + checks: credentialCached((input) => Cache.get(checksCache, refCacheKey(input))), activity: credentialCached(activity), threadComments, diff: credentialCached(diff), diff --git a/apps/server/src/sourceControl/BitbucketApi.test.ts b/apps/server/src/sourceControl/BitbucketApi.test.ts index 856d043a7fce..b27d56bef399 100644 --- a/apps/server/src/sourceControl/BitbucketApi.test.ts +++ b/apps/server/src/sourceControl/BitbucketApi.test.ts @@ -618,6 +618,30 @@ it.effect("preserves Bitbucket response body read failures as their immediate ca }).pipe(Effect.provide(layer)); }); +it.effect("keeps the 429 retry time when the response body cannot be read", () => { + const { layer } = makeLayer({ + response: () => + new Response( + new ReadableStream({ + start: (controller) => controller.error(new Error("response stream failed")), + }), + { status: 429, headers: { "Retry-After": "120" } }, + ), + }); + + return Effect.gen(function* () { + yield* TestClock.setTime(1_000); + const bitbucket = yield* BitbucketApi.BitbucketApi; + const error = yield* bitbucket + .request({ method: "GET", url: "/repositories/acme/web" }) + .pipe(Effect.flip); + + assert.instanceOf(error, BitbucketApi.BitbucketResponseBodyReadError); + assert.strictEqual(error.status, 429); + assert.strictEqual(error.retryAt, 121_000); + }).pipe(Effect.provide(layer)); +}); + it.effect("checks out same-repository pull requests with the existing Bitbucket remote", () => { const { git, layer } = makeLayer({ response: () => diff --git a/apps/server/src/sourceControl/BitbucketApi.ts b/apps/server/src/sourceControl/BitbucketApi.ts index 1223f95f68fd..7eb24096e33b 100644 --- a/apps/server/src/sourceControl/BitbucketApi.ts +++ b/apps/server/src/sourceControl/BitbucketApi.ts @@ -117,6 +117,7 @@ export class BitbucketResponseBodyReadError extends Schema.TaggedError { + const paths: string[] = []; + return Effect.gen(function* () { + const provider = yield* ForgejoPullRequestProvider.make; + const read = provider.getChangeRequestChecks; + if (read === undefined) return yield* Effect.die("checks read missing"); + const result = yield* read({ + cwd: "/repo", + repository: "acme/web", + host: "forgejo.test", + number: 1, + }); + assert.strictEqual(result.state, "open"); + assert.strictEqual(result.checks[0]?.status, "failure"); + assert.deepStrictEqual(paths, [ + "repos/acme/web/pulls/1", + "repos/acme/web/statuses/head?sort=recentupdate&limit=50&page=1", + "repos/acme/web/statuses/head?sort=recentupdate&limit=50&page=2", + ]); + }).pipe( + Effect.provide( + Layer.mock(ForgejoCli.ForgejoCli)({ + api: (input) => { + paths.push(input.path); + assert.match(input.path, /^repos\/acme\/web\/(pulls\/1|statuses\/head)/); + return Effect.succeed( + processOutput( + input.path.endsWith("pulls/1") + ? `{"number":1,"title":"Checks","body":"","html_url":"https://forgejo.test/acme/web/pulls/1", "user":null,"state":"open","merged":false, + "head":{"ref":"feature","sha":"head","repo":null},"base":{"ref":"main","sha":"base","repo":null}, + "created_at":"2026-09-16T00:00:00Z","updated_at":"2026-09-16T00:00:00Z","closed_at":null,"merged_at":null,"labels":[]}` + : input.path.endsWith("page=1") + ? `[{"context":"build","status":"failure","description":null,"target_url":null,"updated_at":"2026-09-16T00:00:00Z"}]` + : "[]", + ), + ); + }, + }), + ), + ); +}); + it.effect("loads Forgejo pull request references from files and commits views", () => Effect.gen(function* () { const provider = yield* ForgejoSourceControlProvider.make; diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts index 2fad569b62f1..3622aa3b37fb 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -2555,6 +2555,12 @@ const makeWsRpcLayer = ( "rpc.aggregate": "pull-requests", }, ), + [WS_METHODS.pullRequestsChecks]: (input) => + observeRpcEffect( + WS_METHODS.pullRequestsChecks, + withPullRequestViewer(input, pullRequests.checks(input)), + { "rpc.aggregate": "pull-requests" }, + ), [WS_METHODS.pullRequestsActivity]: (input) => observeRpcEffect( WS_METHODS.pullRequestsActivity, diff --git a/apps/web/src/components/chat/ThreadDetailsPrRow.test.tsx b/apps/web/src/components/chat/ThreadDetailsPrRow.test.tsx new file mode 100644 index 000000000000..fa93ca35949e --- /dev/null +++ b/apps/web/src/components/chat/ThreadDetailsPrRow.test.tsx @@ -0,0 +1,110 @@ +import { EnvironmentId, type PullRequestCheck } from "@t3tools/contracts"; +import { act, cloneElement, type ReactElement, type ReactNode } from "react"; +import { create, type ReactTestRenderer } from "react-test-renderer"; +import { afterEach, expect, it, vi } from "vite-plus/test"; + +const state = vi.hoisted(() => ({ + status: "success" as PullRequestCheck["status"], + perform: vi.fn(), +})); + +vi.mock("~/state/entities", () => ({ useServerConfigs: () => new Map() })); +vi.mock("~/state/pullRequests", () => ({ pullRequestEnvironment: {} })); +vi.mock("~/hooks/useLiveRefresh", () => ({ useLiveRefresh: () => {} })); +vi.mock("~/state/query", () => ({ + useEnvironmentQuery: () => ({ + data: { + number: 1, + title: "Test PR", + state: "open", + isDraft: false, + mergeability: "mergeable", + headBranch: "feature", + baseBranch: "main", + changedFiles: 1, + additions: 1, + deletions: 0, + checks: [{ name: "CI", status: state.status, description: null, url: null }], + capabilities: { actions: ["merge"], mergeMethods: ["merge"] }, + viewerPermissions: { actions: ["merge"] }, + mergeCapabilities: { merge: true, squash: false, rebase: false }, + }, + dataUpdatedAt: 1, + isPending: false, + refresh: vi.fn(), + }), +})); +vi.mock("../pullRequest/usePullRequestActions", () => ({ + usePullRequestActionRunner: () => ({ actionPending: false, perform: state.perform }), + usePullRequestHandoffs: () => ({ handoff: null, startHandoff: vi.fn() }), +})); +vi.mock("../pullRequest/PullRequestChecksPopover", () => ({ + PullRequestChecksPopover: () => null, +})); +vi.mock("../ui/tooltip", () => ({ + Tooltip: ({ children }: { children: ReactNode }) => children, + TooltipTrigger: ({ render, children }: { render: ReactElement; children: ReactNode }) => + cloneElement(render, undefined, children), + TooltipPopup: () => null, +})); +vi.mock("../ui/alert-dialog", () => ({ + AlertDialog: ({ open, children }: { open: boolean; children: ReactNode }) => + open ?
{children}
: null, + AlertDialogPopup: ({ children }: { children: ReactNode }) => children, + AlertDialogHeader: ({ children }: { children: ReactNode }) => children, + AlertDialogTitle: ({ children }: { children: ReactNode }) =>

{children}

, + AlertDialogDescription: ({ children }: { children: ReactNode }) => children, + AlertDialogFooter: ({ children }: { children: ReactNode }) => children, + AlertDialogClose: () => null, +})); + +import { ThreadDetailsPrRow } from "./ThreadDetailsPrRow"; + +let renderer: ReactTestRenderer; +afterEach(() => { + act(() => renderer?.unmount()); + vi.unstubAllGlobals(); +}); + +it("requires a new merge click after passing checks become pending and pass again", () => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const render = () => ( + + ); + const clickMerge = () => + act(() => { + renderer.root + .findAllByType("button") + .find((button) => button.children.includes("Merge"))! + .props.onClick(); + }); + const dialogs = () => renderer.root.findAllByProps({ role: "alertdialog" }); + + act(() => { + renderer = create(render()); + }); + clickMerge(); + expect(dialogs()).toHaveLength(1); + act(() => { + state.status = "pending"; + renderer.update(render()); + }); + expect(dialogs()).toHaveLength(0); + act(() => { + state.status = "success"; + renderer.update(render()); + }); + expect(dialogs()).toHaveLength(0); + clickMerge(); + expect(dialogs()).toHaveLength(1); + expect(state.perform).not.toHaveBeenCalled(); +}); diff --git a/apps/web/src/components/chat/ThreadDetailsPrRow.tsx b/apps/web/src/components/chat/ThreadDetailsPrRow.tsx index 694e214df513..ba461b592940 100644 --- a/apps/web/src/components/chat/ThreadDetailsPrRow.tsx +++ b/apps/web/src/components/chat/ThreadDetailsPrRow.tsx @@ -16,9 +16,11 @@ import { PullRequestGlyph } from "../pullRequest/pullRequestIcons"; */ import type { EnvironmentProject } from "@t3tools/client-runtime/state/shell"; import type { EnvironmentId, ProjectId, PullRequestRef } from "@t3tools/contracts"; +import { sourceControlRepositorySelector } from "@t3tools/shared/sourceControl"; import { ArrowUpRightIcon, FileDiffIcon, GitBranchIcon, TriangleAlertIcon } from "lucide-react"; import { useState, type MouseEvent as ReactMouseEvent } from "react"; +import { useLiveRefresh } from "~/hooks/useLiveRefresh"; import { cn } from "~/lib/utils"; import { useServerConfigs } from "~/state/entities"; import { pullRequestEnvironment } from "~/state/pullRequests"; @@ -34,7 +36,9 @@ import { allowedPullRequestMergeMethods, resolveThreadPanelPullRequestAction, } from "../pullRequest/pullRequestDetail.logic"; +import { PullRequestChecksPopover } from "../pullRequest/PullRequestChecksPopover"; import { + pullRequestChecksState, PullRequestCheckStatusIcon, PullRequestDiffStat, resolvePullRequestState, @@ -96,11 +100,7 @@ export function ThreadDetailsPrRow({ const serverConfigs = useServerConfigs(); const supportsPullRequests = serverConfigs.get(environmentId)?.environment.capabilities.pullRequests === true; - // The identity's own spelling, the way the detail panel is addressed everywhere else. - const identity = project?.repositoryIdentity; - const repository = - identity?.displayName ?? - (identity?.owner && identity.name ? `${identity.owner}/${identity.name}` : null); + const repository = sourceControlRepositorySelector(project?.repositoryIdentity); const reference: PullRequestRef | null = supportsPullRequests && project !== null ? linkedReference @@ -110,15 +110,45 @@ export function ThreadDetailsPrRow({ : null : null; const detailQuery = useEnvironmentQuery( - reference === null ? null : pullRequestEnvironment.detail({ environmentId, input: reference }), + reference === null + ? null + : pullRequestEnvironment.detail({ + environmentId, + input: { ...reference, allowStale: false }, + }), + ); + const supportsChecks = + serverConfigs.get(environmentId)?.environment.capabilities.pullRequestChecks === true; + const checksQuery = useEnvironmentQuery( + supportsChecks && reference !== null && detailQuery.data !== null + ? pullRequestEnvironment.checks({ environmentId, input: reference }) + : null, ); - const detail = detailQuery.data ?? null; + const detail = + detailQuery.data === null + ? null + : checksQuery.data !== null && checksQuery.dataUpdatedAt >= detailQuery.dataUpdatedAt + ? { ...detailQuery.data, ...checksQuery.data } + : detailQuery.data; + const open = reference !== null && (detail?.state ?? pr?.state) === "open"; + const refreshKey = `${environmentId}:${project?.id}:${reference?.host}:${reference?.repository}:${number}`; + useLiveRefresh(detailQuery.isPending ? null : detailQuery.refresh, { + enabled: open, + key: `workspace-pr:${refreshKey}`, + intervalMs: 10 * 60_000, + }); + useLiveRefresh(checksQuery.isPending || detailQuery.isPending ? null : checksQuery.refresh, { + enabled: open && supportsChecks && !(checksQuery.isSuccess && checksQuery.data === null), + key: `workspace-pr-checks:${refreshKey}`, + intervalMs: 45_000, + }); const { actionPending, perform } = usePullRequestActionRunner({ environmentId, reference, onSuccess: () => { detailQuery.refresh(); + checksQuery.refresh(); onActed?.(); }, }); @@ -126,9 +156,12 @@ export function ThreadDetailsPrRow({ const [confirmingMerge, setConfirmingMerge] = useState(false); const rowAction = resolveThreadPanelPullRequestAction(detail); + if (confirmingMerge && rowAction !== "merge") { + setConfirmingMerge(false); + } const conflicting = isPullRequestConflicting(detail); const checksState = detail === null ? "none" : classifyPullRequestChecks(detail.checks); - const checksRunning = detail?.state === "open" && rowAction === null && checksState === "pending"; + const checksRollup = detail === null ? null : pullRequestChecksState(detail.checks); const selectedMergeMethod = resolveSelectedMergeMethod( allowedPullRequestMergeMethods(detail), "merge", @@ -310,7 +343,7 @@ export function ThreadDetailsPrRow({ return ( <> - {trailingAction || checksRunning ? ( + {detail ? (
{rowTooltip} -
) : ( diff --git a/apps/web/src/components/pullRequest/PullRequestChecksPopover.tsx b/apps/web/src/components/pullRequest/PullRequestChecksPopover.tsx index 20100339ecbb..e00c571daa11 100644 --- a/apps/web/src/components/pullRequest/PullRequestChecksPopover.tsx +++ b/apps/web/src/components/pullRequest/PullRequestChecksPopover.tsx @@ -5,6 +5,8 @@ import type { PullRequestRef, ScopedThreadRef, } from "@t3tools/contracts"; +import { ChevronDownIcon } from "lucide-react"; +import { useState } from "react"; import { useOpenLink } from "~/browser/useOpenLink"; import { cn } from "~/lib/utils"; @@ -12,8 +14,11 @@ import { pullRequestEnvironment } from "~/state/pullRequests"; import { useEnvironmentQuery } from "~/state/query"; import { Popover, PopoverPopup, PopoverTrigger } from "../ui/popover"; +import { Button } from "../ui/button"; +import { ScrollArea } from "../ui/scroll-area"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; import { toastManager } from "../ui/toast"; +import { groupPullRequestChecks } from "./pullRequestDetail.logic"; import { PullRequestCheckStatusIcon, pullRequestCheckStatusLabel, @@ -59,43 +64,61 @@ function ChecksBody({ threadRef: ScopedThreadRef | null; }) { const openLink = useOpenLink(threadRef); + const [showAll, setShowAll] = useState(false); + const { attention, running, completed } = groupPullRequestChecks(checks); + const canCollapse = attention.length + running.length > 0 && completed.length > 0; + const visibleChecks = [...attention, ...running, ...(showAll || !canCollapse ? completed : [])]; if (checks.length === 0) { return

No checks reported

; } return ( -
    - {/* Keyed by position as well as by name: the host is the one that decides how many runs + <> + +
      + {/* Keyed by position as well as by name: the host is the one that decides how many runs share a name, and a repeated key is a rendering fault rather than a wrong list. */} - {checks.map((check, index) => ( -
    • - - - {check.name}} - /> - {check.description ?? check.name} - - - {pullRequestCheckStatusLabel(check)} - - {check.url === null ? null : ( - - )} -
      • - ))} -
    + {visibleChecks.map((check, index) => ( +
  • + + + {check.name}} + /> + {check.description ?? check.name} + + + {pullRequestCheckStatusLabel(check)} + + {check.url === null ? null : ( + + )} +
    • + ))} +
+ + {canCollapse ? ( + + ) : null} + ); } @@ -112,6 +135,7 @@ export function PullRequestChecksPopover({ environmentId, reference, threadRef = null, + variant = "icon", className, }: { checksState: PullRequestChecksState; @@ -121,29 +145,55 @@ export function PullRequestChecksPopover({ reference?: PullRequestRef; /** Thread the popover sits beside; a listing row has none. */ threadRef?: ScopedThreadRef | null; + variant?: "icon" | "count"; className?: string; }) { const presentation = pullRequestChecksStatePresentation(checksState); // Counts beat the rollup's own wording where they are known, the way GitHub's own header reads. const summary = checks === undefined ? null : summarizePullRequestChecks(checks); + const count = checks?.filter((check) => + checksState === "failing" + ? check.status === "failure" || check.status === "cancelled" + : checksState === "pending" + ? check.status === "pending" || check.status === "action-required" + : check.status === "success", + ).length; return ( {/* A listing row is itself a button, so the trigger renders as a span: a nested button is not valid inside one. The click is stopped here so opening the checks does not also select the row it sits on. */} + variant === "count" ? ( +