| author | |
| committer | |
| log | ba46f922a271942d42faf305ff833af89cb5c3a1 |
| tree | 272dfdfeddc292d8e6d5940a47c497260c01d666 |
| parent | fee10def1afb63cecd9a626aeba2bb063828cf22 |
| signature | Signed by PGP key B5690EEEBB952194 |
41 files changed, 1781 insertions(+), 192 deletions(-)
apps/server/src/auth/RpcAuthorization.ts+1| ... | ... | @@ -56,6 +56,7 @@ export const RPC_REQUIRED_SCOPES = { |
| 56 | 56 | [WS_METHODS.pullRequestsListStats]: AuthOrchestrationReadScope, |
| 57 | 57 | [WS_METHODS.pullRequestsDetail]: AuthOrchestrationReadScope, |
| 58 | 58 | [WS_METHODS.pullRequestsActivity]: AuthOrchestrationReadScope, |
| 59 | [WS_METHODS.pullRequestsThreadComments]: AuthOrchestrationReadScope, | |
| 59 | 60 | [WS_METHODS.pullRequestsDiffFileContents]: AuthOrchestrationReadScope, |
| 60 | 61 | [WS_METHODS.pullRequestsRunAction]: AuthOrchestrationOperateScope, |
| 61 | 62 | [WS_METHODS.pullRequestsUpdate]: AuthOrchestrationOperateScope, |
apps/server/src/pullRequest/AzureDevOpsPullRequestProvider.ts+8-6| ... | ... | @@ -4,6 +4,7 @@ import type { PullRequestCapabilities, PullRequestViewerPermissions } from "@t3t |
| 4 | 4 | import * as AzureDevOpsPullRequestCli from "./AzureDevOpsPullRequestCli.ts"; |
| 5 | 5 | import { |
| 6 | 6 | PullRequestProviderError, |
| 7 | type PullRequestProviderFailure, | |
| 7 | 8 | type ProviderChangeRequest, |
| 8 | 9 | type ProviderChangeRequestActivity, |
| 9 | 10 | type ProviderChangeRequestDetail, |
| ... | ... | @@ -64,12 +65,13 @@ export const AZURE_DEVOPS_VIEWER_PERMISSIONS: PullRequestViewerPermissions = { |
| 64 | 65 | }; |
| 65 | 66 | |
| 66 | 67 | /** The CLI tags that mean the tool itself is unusable, rather than one request failing. */ |
| 67 | function reasonFor( | |
| 68 | export function azureDevOpsProviderFailure( | |
| 68 | 69 | error: AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCliError, |
| 69 | ): PullRequestProviderError["reason"] { | |
| 70 | if (error._tag === "AzureDevOpsCliUnavailableError") return "missing-tool"; | |
| 71 | if (error._tag === "AzureDevOpsCliAuthenticationError") return "unauthenticated"; | |
| 72 | return "failed"; | |
| 70 | ): PullRequestProviderFailure { | |
| 71 | if (error._tag === "AzureDevOpsCliUnavailableError") return { reason: "missing-tool" }; | |
| 72 | if (error._tag === "AzureDevOpsCliAuthenticationError") return { reason: "unauthenticated" }; | |
| 73 | if (error._tag === "AzureDevOpsCliRateLimitError") return { reason: "rate-limited" }; | |
| 74 | return { reason: "failed" }; | |
| 73 | 75 | } |
| 74 | 76 | |
| 75 | 77 | function toChangeRequest(pullRequest: AzureDevOpsPullRequest): ProviderChangeRequest { |
| ... | ... | @@ -103,7 +105,7 @@ export const make = Effect.gen(function* () { |
| 103 | 105 | new PullRequestProviderError({ |
| 104 | 106 | provider: "azure-devops", |
| 105 | 107 | operation, |
| 106 | reason: reasonFor(error), | |
| 108 | ...azureDevOpsProviderFailure(error), | |
| 107 | 109 | detail: error.detail, |
| 108 | 110 | cause: error, |
| 109 | 111 | }); |
apps/server/src/pullRequest/BitbucketPullRequestProvider.test.ts+4-4| ... | ... | @@ -2,11 +2,11 @@ import { describe, expect, it } from "vite-plus/test"; |
| 2 | 2 | |
| 3 | 3 | import * as BitbucketApi from "../sourceControl/BitbucketApi.ts"; |
| 4 | 4 | import { |
| 5 | bitbucketErrorReason, | |
| 5 | bitbucketProviderFailure, | |
| 6 | 6 | bitbucketViewerPermissions, |
| 7 | 7 | } from "./BitbucketPullRequestProvider.ts"; |
| 8 | 8 | |
| 9 | describe("bitbucketErrorReason", () => { | |
| 9 | describe("bitbucketProviderFailure", () => { | |
| 10 | 10 | it("treats only an HTTP 401 as unusable credentials", () => { |
| 11 | 11 | const responseError = (status: number) => |
| 12 | 12 | new BitbucketApi.BitbucketResponseError({ |
| ... | ... | @@ -15,8 +15,8 @@ describe("bitbucketErrorReason", () => { |
| 15 | 15 | responseBodyLength: 0, |
| 16 | 16 | }); |
| 17 | 17 | |
| 18 | expect(bitbucketErrorReason(responseError(401))).toBe("unauthenticated"); | |
| 19 | expect(bitbucketErrorReason(responseError(403))).toBe("failed"); | |
| 18 | expect(bitbucketProviderFailure(responseError(401)).reason).toBe("unauthenticated"); | |
| 19 | expect(bitbucketProviderFailure(responseError(403)).reason).toBe("failed"); | |
| 20 | 20 | }); |
| 21 | 21 | }); |
| 22 | 22 |
apps/server/src/pullRequest/BitbucketPullRequestProvider.ts+12-5| ... | ... | @@ -4,6 +4,7 @@ import type { PullRequestCapabilities, PullRequestViewerPermissions } from "@t3t |
| 4 | 4 | import * as BitbucketPullRequestApi from "./BitbucketPullRequestApi.ts"; |
| 5 | 5 | import { |
| 6 | 6 | PullRequestProviderError, |
| 7 | type PullRequestProviderFailure, | |
| 7 | 8 | type ProviderChangeRequest, |
| 8 | 9 | type ProviderChangeRequestActivity, |
| 9 | 10 | type ProviderChangeRequestDetail, |
| ... | ... | @@ -59,15 +60,21 @@ export function bitbucketViewerPermissions(input: { |
| 59 | 60 | } |
| 60 | 61 | |
| 61 | 62 | /** The failures that mean the credentials are the problem, rather than one request. */ |
| 62 | export function bitbucketErrorReason( | |
| 63 | export function bitbucketProviderFailure( | |
| 63 | 64 | error: BitbucketPullRequestApi.BitbucketPullRequestApiError, |
| 64 | ): PullRequestProviderError["reason"] { | |
| 65 | ): PullRequestProviderFailure { | |
| 65 | 66 | // Bitbucket is read over HTTP with credentials from the environment, so there is no tool to be |
| 66 | 67 | // missing: unusable always means the credentials are absent or refused. |
| 67 | 68 | if (error._tag === "BitbucketResponseError" && error.status === 401) { |
| 68 | return "unauthenticated"; | |
| 69 | return { reason: "unauthenticated" }; | |
| 69 | 70 | } |
| 70 | return "failed"; | |
| 71 | if (error._tag === "BitbucketResponseError" && error.status === 429) { | |
| 72 | return { | |
| 73 | reason: "rate-limited", | |
| 74 | ...(error.retryAt === undefined ? {} : { retryAt: error.retryAt }), | |
| 75 | }; | |
| 76 | } | |
| 77 | return { reason: "failed" }; | |
| 71 | 78 | } |
| 72 | 79 | |
| 73 | 80 | function toChangeRequest(pullRequest: BitbucketPullRequest): ProviderChangeRequest { |
| ... | ... | @@ -100,7 +107,7 @@ export const make = Effect.gen(function* () { |
| 100 | 107 | new PullRequestProviderError({ |
| 101 | 108 | provider: "bitbucket", |
| 102 | 109 | operation, |
| 103 | reason: bitbucketErrorReason(error), | |
| 110 | ...bitbucketProviderFailure(error), | |
| 104 | 111 | // Every Bitbucket failure states its own fact; this names the operation around it, so |
| 105 | 112 | // the two do not stack into "failed in x: failed in y: ...". |
| 106 | 113 | detail: error.detail, |
apps/server/src/pullRequest/GitHubPullRequestCli.test.ts+216-10| ... | ... | @@ -1,9 +1,11 @@ |
| 1 | 1 | import { afterEach, assert, expect, it, vi } from "@effect/vitest"; |
| 2 | 2 | import * as Effect from "effect/Effect"; |
| 3 | 3 | import * as Layer from "effect/Layer"; |
| 4 | import * as TestClock from "effect/testing/TestClock"; | |
| 4 | 5 | import { ChildProcessSpawner } from "effect/unstable/process"; |
| 5 | 6 | |
| 6 | 7 | import * as GitHubCli from "../sourceControl/GitHubCli.ts"; |
| 8 | import * as GitHubGraphQlBudget from "../sourceControl/githubGraphQlBudget.ts"; | |
| 7 | 9 | import * as GitHubPullRequestCli from "./GitHubPullRequestCli.ts"; |
| 8 | 10 | import { BASE_COMPARISON_GRAPHQL_QUERY } from "./gitHubPullRequestJson.ts"; |
| 9 | 11 | |
| ... | ... | @@ -16,6 +18,7 @@ const layer = it.layer( |
| 16 | 18 | execute: mockedExecute, |
| 17 | 19 | }), |
| 18 | 20 | ), |
| 21 | Layer.provide(GitHubGraphQlBudget.layer), | |
| 19 | 22 | ), |
| 20 | 23 | ); |
| 21 | 24 | |
| ... | ... | @@ -107,9 +110,16 @@ function threadCommentsPage( |
| 107 | 110 | ids: ReadonlyArray<string>, |
| 108 | 111 | endCursor: string | null, |
| 109 | 112 | totalCount: number, |
| 113 | pullRequestId = "PR_7", | |
| 110 | 114 | ): string { |
| 111 | 115 | return JSON.stringify({ |
| 112 | data: { node: { comments: threadComments(ids, endCursor, totalCount) } }, | |
| 116 | data: { | |
| 117 | repository: { pullRequest: { id: "PR_7" } }, | |
| 118 | node: { | |
| 119 | pullRequest: { id: pullRequestId }, | |
| 120 | comments: threadComments(ids, endCursor, totalCount), | |
| 121 | }, | |
| 122 | }, | |
| 113 | 123 | }); |
| 114 | 124 | } |
| 115 | 125 | |
| ... | ... | @@ -1601,6 +1611,41 @@ layer("GitHubPullRequestCli.layer", (it) => { |
| 1601 | 1611 | }), |
| 1602 | 1612 | ); |
| 1603 | 1613 | |
| 1614 | it.effect("accounts for the avatar lookup in the GraphQL budget", () => | |
| 1615 | Effect.gen(function* () { | |
| 1616 | mockedExecute.mockReturnValueOnce( | |
| 1617 | Effect.succeed( | |
| 1618 | output( | |
| 1619 | // @effect-diagnostics-next-line preferSchemaOverJson:off | |
| 1620 | JSON.stringify({ | |
| 1621 | data: { | |
| 1622 | nodes: [{ login: "octocat", avatarUrl: "https://avatars/octocat" }], | |
| 1623 | rateLimit: { | |
| 1624 | cost: 1, | |
| 1625 | limit: 5_000, | |
| 1626 | remaining: 4_999, | |
| 1627 | resetAt: "2099-08-13T14:00:00Z", | |
| 1628 | }, | |
| 1629 | }, | |
| 1630 | }), | |
| 1631 | ), | |
| 1632 | ), | |
| 1633 | ); | |
| 1634 | const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; | |
| 1635 | ||
| 1636 | const avatars = yield* cli.listActorAvatars({ | |
| 1637 | cwd: "/w", | |
| 1638 | repository: "acme/web", | |
| 1639 | host: "github.com", | |
| 1640 | ids: ["MDQ6VXNlcjE="], | |
| 1641 | }); | |
| 1642 | ||
| 1643 | expect(callAt(0).args).toContain("ids[]=MDQ6VXNlcjE="); | |
| 1644 | expect(callAt(0).args.at(-1)).toContain("rateLimit { cost limit remaining resetAt }"); | |
| 1645 | expect(avatars.get("octocat")).toBe("https://avatars/octocat"); | |
| 1646 | }), | |
| 1647 | ); | |
| 1648 | ||
| 1604 | 1649 | it.effect("fails when the authenticated account has no login", () => |
| 1605 | 1650 | Effect.gen(function* () { |
| 1606 | 1651 | mockedExecute.mockReturnValueOnce(Effect.succeed(output(" "))); |
| ... | ... | @@ -2144,7 +2189,7 @@ layer("GitHubPullRequestCli.layer", (it) => { |
| 2144 | 2189 | }), |
| 2145 | 2190 | ); |
| 2146 | 2191 | |
| 2147 | it.effect("finishes a thread longer than one page from the thread's own node", () => | |
| 2192 | it.effect("leaves a long thread paged until the reader asks for more", () => | |
| 2148 | 2193 | Effect.gen(function* () { |
| 2149 | 2194 | mockedExecute.mockReturnValueOnce( |
| 2150 | 2195 | Effect.succeed( |
| ... | ... | @@ -2156,22 +2201,71 @@ layer("GitHubPullRequestCli.layer", (it) => { |
| 2156 | 2201 | ), |
| 2157 | 2202 | ), |
| 2158 | 2203 | ); |
| 2204 | const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; | |
| 2205 | ||
| 2206 | const conversation = yield* cli.listReviewThreadComments({ | |
| 2207 | cwd: "/w", | |
| 2208 | repository: "acme/web", | |
| 2209 | host: "github.com", | |
| 2210 | number: 7, | |
| 2211 | }); | |
| 2212 | ||
| 2213 | assert.strictEqual(mockedExecute.mock.calls.length, 1); | |
| 2214 | expect(conversation.comments.map((comment) => comment.id)).toEqual(["c1"]); | |
| 2215 | expect(conversation.reviewThreads[0]).toMatchObject({ | |
| 2216 | commentCount: 3, | |
| 2217 | nextCommentsCursor: "Y3Vyc29yOjI", | |
| 2218 | }); | |
| 2219 | assert.isTrue(conversation.truncated); | |
| 2220 | }), | |
| 2221 | ); | |
| 2222 | ||
| 2223 | it.effect("reads one requested page from a review thread cursor", () => | |
| 2224 | Effect.gen(function* () { | |
| 2159 | 2225 | mockedExecute.mockReturnValueOnce( |
| 2160 | 2226 | Effect.succeed(output(threadCommentsPage(["c2", "c3"], null, 3))), |
| 2161 | 2227 | ); |
| 2162 | 2228 | const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; |
| 2163 | 2229 | |
| 2164 | const conversation = yield* cli.listReviewThreadComments({ | |
| 2230 | const page = yield* cli.getReviewThreadComments({ | |
| 2165 | 2231 | cwd: "/w", |
| 2166 | 2232 | repository: "acme/web", |
| 2167 | 2233 | host: "github.com", |
| 2168 | 2234 | number: 7, |
| 2235 | threadId: "PRRT_1", | |
| 2236 | cursor: "Y3Vyc29yOjI", | |
| 2169 | 2237 | }); |
| 2170 | 2238 | |
| 2171 | expect(callAt(1).args).toContain("threadId=PRRT_1"); | |
| 2172 | expect(conversation.comments.map((comment) => comment.id)).toEqual(["c1", "c2", "c3"]); | |
| 2173 | // GitHub's own count, which is what the page shows however much of it was read. | |
| 2174 | assert.strictEqual(conversation.commentCount, 3); | |
| 2239 | expect(callAt(0).args).toContain("owner=acme"); | |
| 2240 | expect(callAt(0).args).toContain("name=web"); | |
| 2241 | expect(callAt(0).args).toContain("number=7"); | |
| 2242 | expect(callAt(0).args).toContain("threadId=PRRT_1"); | |
| 2243 | expect(callAt(0).args).toContain("cursor=Y3Vyc29yOjI"); | |
| 2244 | assert.strictEqual(mockedExecute.mock.calls.length, 1); | |
| 2245 | expect(page.comments.map((comment) => comment.id)).toEqual(["c2", "c3"]); | |
| 2246 | expect(page.nextCursor).toBeNull(); | |
| 2247 | }), | |
| 2248 | ); | |
| 2249 | ||
| 2250 | it.effect("refuses a review thread from another pull request", () => | |
| 2251 | Effect.gen(function* () { | |
| 2252 | mockedExecute.mockReturnValueOnce( | |
| 2253 | Effect.succeed(output(threadCommentsPage(["foreign"], null, 1, "PR_8"))), | |
| 2254 | ); | |
| 2255 | const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; | |
| 2256 | ||
| 2257 | const error = yield* Effect.flip( | |
| 2258 | cli.getReviewThreadComments({ | |
| 2259 | cwd: "/w", | |
| 2260 | repository: "acme/web", | |
| 2261 | host: "github.com", | |
| 2262 | number: 7, | |
| 2263 | threadId: "PRRT_FOREIGN", | |
| 2264 | cursor: "Y3Vyc29yOjI", | |
| 2265 | }), | |
| 2266 | ); | |
| 2267 | ||
| 2268 | assert.strictEqual(error._tag, "GitHubSubjectScopeError"); | |
| 2175 | 2269 | }), |
| 2176 | 2270 | ); |
| 2177 | 2271 | |
| ... | ... | @@ -2242,7 +2336,7 @@ layer("GitHubPullRequestCli.layer", (it) => { |
| 2242 | 2336 | // The tuples are flattened straight into argv, so a variable without its flag is a |
| 2243 | 2337 | // positional argument gh refuses outright. |
| 2244 | 2338 | const args = callAt(0).args; |
| 2245 | expect(args).toEqual([ | |
| 2339 | expect(args.slice(0, -2)).toEqual([ | |
| 2246 | 2340 | "api", |
| 2247 | 2341 | "graphql", |
| 2248 | 2342 | "--hostname", |
| ... | ... | @@ -2255,10 +2349,122 @@ layer("GitHubPullRequestCli.layer", (it) => { |
| 2255 | 2349 | "number=7", |
| 2256 | 2350 | "-f", |
| 2257 | 2351 | "headRef=fork:feat/page", |
| 2258 | "-f", | |
| 2259 | `query=${BASE_COMPARISON_GRAPHQL_QUERY}`, | |
| 2260 | 2352 | ]); |
| 2261 | 2353 | expect(comparison).toEqual({ behindBy: 4, viewerCanUpdate: true }); |
| 2354 | expect(args.at(-2)).toBe("-f"); | |
| 2355 | expect(args.at(-1)).toContain(`query=${BASE_COMPARISON_GRAPHQL_QUERY.slice(0, -2)}`); | |
| 2356 | }), | |
| 2357 | ); | |
| 2358 | ||
| 2359 | it.effect("stops GraphQL reads at the protected reserve until reset", () => | |
| 2360 | Effect.gen(function* () { | |
| 2361 | mockedExecute.mockReturnValue( | |
| 2362 | Effect.succeed( | |
| 2363 | output( | |
| 2364 | // @effect-diagnostics-next-line preferSchemaOverJson:off | |
| 2365 | JSON.stringify({ | |
| 2366 | data: { | |
| 2367 | repository: { | |
| 2368 | pullRequest: { | |
| 2369 | viewerCanUpdateBranch: true, | |
| 2370 | baseRef: { compare: { behindBy: 4 } }, | |
| 2371 | }, | |
| 2372 | }, | |
| 2373 | rateLimit: { | |
| 2374 | cost: 1, | |
| 2375 | limit: 5_000, | |
| 2376 | remaining: 500, | |
| 2377 | resetAt: "2099-08-13T14:00:00Z", | |
| 2378 | }, | |
| 2379 | }, | |
| 2380 | }), | |
| 2381 | ), | |
| 2382 | ), | |
| 2383 | ); | |
| 2384 | const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; | |
| 2385 | const input = { | |
| 2386 | cwd: "/w", | |
| 2387 | repository: "acme/web", | |
| 2388 | host: "github.com", | |
| 2389 | number: 7, | |
| 2390 | headRef: "fork:feat/page", | |
| 2391 | } as const; | |
| 2392 | ||
| 2393 | yield* cli.getPullRequestBaseComparison(input); | |
| 2394 | expect(callAt(0).args.at(-1)).toContain("rateLimit { cost limit remaining resetAt }"); | |
| 2395 | ||
| 2396 | const error = yield* Effect.flip(cli.getPullRequestBaseComparison(input)); | |
| 2397 | ||
| 2398 | assert.strictEqual(error._tag, "SourceControlRateLimitPausedError"); | |
| 2399 | if (error._tag !== "SourceControlRateLimitPausedError") return; | |
| 2400 | assert.strictEqual(error.host, "github.com"); | |
| 2401 | assert.strictEqual(error.retryAt, Date.parse("2099-08-13T14:00:00Z")); | |
| 2402 | assert.strictEqual(mockedExecute.mock.calls.length, 1); | |
| 2403 | yield* TestClock.setTime(Date.parse("2100-01-01T00:00:00Z")); | |
| 2404 | }), | |
| 2405 | ); | |
| 2406 | ||
| 2407 | it.effect("lets an interactive permission read use the protected reserve", () => | |
| 2408 | Effect.gen(function* () { | |
| 2409 | mockedExecute | |
| 2410 | .mockReturnValueOnce( | |
| 2411 | Effect.succeed( | |
| 2412 | output( | |
| 2413 | // @effect-diagnostics-next-line preferSchemaOverJson:off | |
| 2414 | JSON.stringify({ | |
| 2415 | data: { | |
| 2416 | repository: { | |
| 2417 | pullRequest: { | |
| 2418 | viewerCanUpdateBranch: true, | |
| 2419 | baseRef: { compare: { behindBy: 4 } }, | |
| 2420 | }, | |
| 2421 | }, | |
| 2422 | rateLimit: { | |
| 2423 | cost: 1, | |
| 2424 | limit: 5_000, | |
| 2425 | remaining: 500, | |
| 2426 | resetAt: "2099-08-13T14:00:00Z", | |
| 2427 | }, | |
| 2428 | }, | |
| 2429 | }), | |
| 2430 | ), | |
| 2431 | ), | |
| 2432 | ) | |
| 2433 | .mockReturnValueOnce( | |
| 2434 | Effect.succeed( | |
| 2435 | output( | |
| 2436 | // @effect-diagnostics-next-line preferSchemaOverJson:off | |
| 2437 | JSON.stringify({ | |
| 2438 | data: { | |
| 2439 | repository: { | |
| 2440 | viewerPermission: "READ", | |
| 2441 | pullRequest: { viewerCanUpdate: true, viewerDidAuthor: true }, | |
| 2442 | }, | |
| 2443 | }, | |
| 2444 | }), | |
| 2445 | ), | |
| 2446 | ), | |
| 2447 | ); | |
| 2448 | const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; | |
| 2449 | ||
| 2450 | yield* cli.getPullRequestBaseComparison({ | |
| 2451 | cwd: "/w", | |
| 2452 | repository: "acme/web", | |
| 2453 | host: "github.com", | |
| 2454 | number: 7, | |
| 2455 | headRef: "fork:feat/page", | |
| 2456 | }); | |
| 2457 | const access = yield* cli.getViewerAccess({ | |
| 2458 | cwd: "/w", | |
| 2459 | repository: "acme/web", | |
| 2460 | host: "github.com", | |
| 2461 | number: 7, | |
| 2462 | allowReserve: true, | |
| 2463 | }); | |
| 2464 | ||
| 2465 | assert.strictEqual(mockedExecute.mock.calls.length, 2); | |
| 2466 | expect(access).toEqual({ canWrite: false, canUpdate: true, didAuthor: true }); | |
| 2467 | yield* TestClock.setTime(Date.parse("2100-01-01T00:00:00Z")); | |
| 2262 | 2468 | }), |
| 2263 | 2469 | ); |
| 2264 | 2470 |
apps/server/src/pullRequest/GitHubPullRequestCli.ts+104-108| ... | ... | @@ -18,11 +18,13 @@ import { |
| 18 | 18 | type PullRequestReviewVerdict, |
| 19 | 19 | type PullRequestReviewerCandidateList, |
| 20 | 20 | type PullRequestReviewerKind, |
| 21 | type PullRequestThreadComment, | |
| 21 | type PullRequestThreadCommentsResult, | |
| 22 | 22 | type PullRequestUpdateMethod, |
| 23 | 23 | } from "@t3tools/contracts"; |
| 24 | 24 | |
| 25 | 25 | import * as GitHubCli from "../sourceControl/GitHubCli.ts"; |
| 26 | import * as GitHubGraphQlBudget from "../sourceControl/githubGraphQlBudget.ts"; | |
| 27 | import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; | |
| 26 | 28 | import { |
| 27 | 29 | ACTOR_AVATARS_GRAPHQL_QUERY, |
| 28 | 30 | ADD_REACTION_GRAPHQL_MUTATION, |
| ... | ... | @@ -247,6 +249,7 @@ export type GitHubPullRequestCliError = |
| 247 | 249 | | GitHubDiffFileContentsUnavailableError |
| 248 | 250 | | GitHubRepositorySelectorError |
| 249 | 251 | | GitHubSubjectScopeError |
| 252 | | SourceControlRateLimit.SourceControlRateLimitPausedError | |
| 250 | 253 | | GitHubViewerLoginUnavailableError; |
| 251 | 254 | |
| 252 | 255 | /** A large pull request can produce a multi-megabyte patch; past this it is truncated. */ |
| ... | ... | @@ -268,15 +271,6 @@ const DIFF_FILES_PAGE_SIZE = 100; |
| 268 | 271 | */ |
| 269 | 272 | const REVIEW_THREAD_PAGES = 10; |
| 270 | 273 | |
| 271 | /** | |
| 272 | * And pages of one thread's own comments, for the rare thread longer than a single page. A | |
| 273 | * thousand replies under one line is already a conversation nobody finishes reading. | |
| 274 | */ | |
| 275 | const REVIEW_THREAD_COMMENT_PAGES = 10; | |
| 276 | ||
| 277 | /** How many over-long threads are finished at once, so a wide conversation is not read serially. */ | |
| 278 | const REVIEW_THREAD_CONCURRENCY = 4; | |
| 279 | ||
| 280 | 274 | export interface GitHubPullRequestListBatch { |
| 281 | 275 | readonly items: ReadonlyArray<GitHubPullRequestListItem>; |
| 282 | 276 | readonly truncated: boolean; |
| ... | ... | @@ -385,6 +379,8 @@ export class GitHubPullRequestCli extends Context.Service< |
| 385 | 379 | readonly number: number; |
| 386 | 380 | /** Qualified `owner:branch`, which is the only form a fork's head resolves under. */ |
| 387 | 381 | readonly headRef: string; |
| 382 | /** Manual action checks may use the quota held back from automatic reads. */ | |
| 383 | readonly allowReserve?: boolean | undefined; | |
| 388 | 384 | }) => Effect.Effect<GitHubBaseComparison, GitHubPullRequestCliError>; |
| 389 | 385 | |
| 390 | 386 | readonly getPullRequestActivity: (input: { |
| ... | ... | @@ -434,6 +430,15 @@ export class GitHubPullRequestCli extends Context.Service< |
| 434 | 430 | readonly ids: ReadonlyArray<string>; |
| 435 | 431 | }) => Effect.Effect<ReadonlyMap<string, string>, GitHubPullRequestCliError>; |
| 436 | 432 | |
| 433 | readonly getReviewThreadComments: (input: { | |
| 434 | readonly cwd: string; | |
| 435 | readonly repository: string; | |
| 436 | readonly host: string; | |
| 437 | readonly number: number; | |
| 438 | readonly threadId: string; | |
| 439 | readonly cursor: string; | |
| 440 | }) => Effect.Effect<PullRequestThreadCommentsResult, GitHubPullRequestCliError>; | |
| 441 | ||
| 437 | 442 | /** One `gh repo view`, which answers what the repository allows and where the viewer stands. */ |
| 438 | 443 | readonly getRepositoryAccess: (input: { |
| 439 | 444 | readonly cwd: string; |
| ... | ... | @@ -447,6 +452,8 @@ export class GitHubPullRequestCli extends Context.Service< |
| 447 | 452 | readonly repository: string; |
| 448 | 453 | readonly host: string; |
| 449 | 454 | readonly number: number; |
| 455 | /** Manual action checks may use the quota held back from automatic reads. */ | |
| 456 | readonly allowReserve?: boolean | undefined; | |
| 450 | 457 | }) => Effect.Effect<GitHubViewerAccess, GitHubPullRequestCliError>; |
| 451 | 458 | |
| 452 | 459 | /** Who this pull request may be sent to, and who it has already been sent to. */ |
| ... | ... | @@ -824,6 +831,7 @@ function actionArgs( |
| 824 | 831 | |
| 825 | 832 | export const make = Effect.gen(function* () { |
| 826 | 833 | const github = yield* GitHubCli.GitHubCli; |
| 834 | const graphQlBudget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; | |
| 827 | 835 | |
| 828 | 836 | /** |
| 829 | 837 | * The pull request's own node id, which is what a mutation against the pull request itself is |
| ... | ... | @@ -841,6 +849,7 @@ export const make = Effect.gen(function* () { |
| 841 | 849 | cwd: input.cwd, |
| 842 | 850 | host: input.host, |
| 843 | 851 | operation: input.operation, |
| 852 | allowReserve: true, | |
| 844 | 853 | variables: [ |
| 845 | 854 | ["-f", `owner=${owner}`], |
| 846 | 855 | ["-f", `name=${name}`], |
| ... | ... | @@ -870,6 +879,7 @@ export const make = Effect.gen(function* () { |
| 870 | 879 | cwd: input.cwd, |
| 871 | 880 | host: input.host, |
| 872 | 881 | operation: input.operation, |
| 882 | allowReserve: true, | |
| 873 | 883 | variables: [ |
| 874 | 884 | ["-f", `owner=${owner}`], |
| 875 | 885 | ["-f", `name=${name}`], |
| ... | ... | @@ -916,6 +926,7 @@ export const make = Effect.gen(function* () { |
| 916 | 926 | readonly cwd: string; |
| 917 | 927 | readonly host: string; |
| 918 | 928 | readonly operation: string; |
| 929 | readonly allowReserve?: boolean | undefined; | |
| 919 | 930 | /** Variables as `-f` flags, for values this module composed itself. */ |
| 920 | 931 | readonly variables?: ReadonlyArray<readonly [string, string]>; |
| 921 | 932 | /** |
| ... | ... | @@ -926,32 +937,40 @@ export const make = Effect.gen(function* () { |
| 926 | 937 | readonly privateVariables?: Readonly<Record<string, string>>; |
| 927 | 938 | readonly query: string; |
| 928 | 939 | readonly decode: (raw: string) => Result.Result<A, unknown>; |
| 929 | }): Effect.Effect<A, GitHubPullRequestCliError> => | |
| 930 | github | |
| 931 | .execute( | |
| 932 | input.privateVariables === undefined | |
| 933 | ? { | |
| 934 | cwd: input.cwd, | |
| 935 | args: [ | |
| 936 | "api", | |
| 937 | "graphql", | |
| 938 | "--hostname", | |
| 939 | input.host, | |
| 940 | ...(input.variables ?? []).flat(), | |
| 941 | "-f", | |
| 942 | `query=${input.query}`, | |
| 943 | ], | |
| 944 | } | |
| 945 | : { | |
| 946 | cwd: input.cwd, | |
| 947 | args: ["api", "graphql", "--hostname", input.host, "--input", "-"], | |
| 948 | stdin: encodeGraphQlRequestJson({ | |
| 949 | query: input.query, | |
| 950 | variables: input.privateVariables, | |
| 951 | }), | |
| 952 | }, | |
| 940 | }): Effect.Effect<A, GitHubPullRequestCliError> => { | |
| 941 | return graphQlBudget | |
| 942 | .query( | |
| 943 | input.host, | |
| 944 | input.query, | |
| 945 | input.allowReserve === true ? { allowReserve: true } : undefined, | |
| 953 | 946 | ) |
| 954 | 947 | .pipe( |
| 948 | Effect.flatMap((query) => | |
| 949 | github.execute( | |
| 950 | input.privateVariables === undefined | |
| 951 | ? { | |
| 952 | cwd: input.cwd, | |
| 953 | args: [ | |
| 954 | "api", | |
| 955 | "graphql", | |
| 956 | "--hostname", | |
| 957 | input.host, | |
| 958 | ...(input.variables ?? []).flat(), | |
| 959 | "-f", | |
| 960 | `query=${query}`, | |
| 961 | ], | |
| 962 | } | |
| 963 | : { | |
| 964 | cwd: input.cwd, | |
| 965 | args: ["api", "graphql", "--hostname", input.host, "--input", "-"], | |
| 966 | stdin: encodeGraphQlRequestJson({ | |
| 967 | query, | |
| 968 | variables: input.privateVariables, | |
| 969 | }), | |
| 970 | }, | |
| 971 | ), | |
| 972 | ), | |
| 973 | Effect.tap((result) => graphQlBudget.observe(input.host, result.stdout)), | |
| 955 | 974 | Effect.flatMap((result) => { |
| 956 | 975 | const decoded = input.decode(result.stdout.trim()); |
| 957 | 976 | return Result.isSuccess(decoded) |
| ... | ... | @@ -966,6 +985,7 @@ export const make = Effect.gen(function* () { |
| 966 | 985 | ); |
| 967 | 986 | }), |
| 968 | 987 | ); |
| 988 | }; | |
| 969 | 989 | |
| 970 | 990 | /** |
| 971 | 991 | * One page of the patch, read from the files API. GitHub refuses `pr diff` outright past 300 |
| ... | ... | @@ -1341,6 +1361,7 @@ export const make = Effect.gen(function* () { |
| 1341 | 1361 | cwd: input.cwd, |
| 1342 | 1362 | host: input.host, |
| 1343 | 1363 | operation: "getPullRequestBaseComparison", |
| 1364 | ...(input.allowReserve === true ? { allowReserve: true } : {}), | |
| 1344 | 1365 | variables: [ |
| 1345 | 1366 | ["-f", `owner=${owner}`], |
| 1346 | 1367 | ["-f", `name=${name}`], |
| ... | ... | @@ -1438,6 +1459,36 @@ export const make = Effect.gen(function* () { |
| 1438 | 1459 | |
| 1439 | 1460 | getPullRequestDiffFileContents, |
| 1440 | 1461 | |
| 1462 | getReviewThreadComments: (input) => { | |
| 1463 | const { owner, name } = parseRepositorySelector(input.repository); | |
| 1464 | return graphqlRead({ | |
| 1465 | cwd: input.cwd, | |
| 1466 | host: input.host, | |
| 1467 | operation: "getReviewThreadComments", | |
| 1468 | variables: [ | |
| 1469 | ["-f", `owner=${owner}`], | |
| 1470 | ["-f", `name=${name}`], | |
| 1471 | ["-F", `number=${input.number}`], | |
| 1472 | ["-f", `threadId=${input.threadId}`], | |
| 1473 | cursorVariable(input.cursor), | |
| 1474 | ], | |
| 1475 | query: REVIEW_THREAD_COMMENTS_GRAPHQL_QUERY, | |
| 1476 | decode: decodeReviewThreadCommentsJson, | |
| 1477 | }).pipe( | |
| 1478 | Effect.flatMap(({ belongsToPullRequest, comments, nextCursor }) => | |
| 1479 | belongsToPullRequest | |
| 1480 | ? Effect.succeed({ comments, nextCursor }) | |
| 1481 | : Effect.fail( | |
| 1482 | new GitHubSubjectScopeError({ | |
| 1483 | command: "gh", | |
| 1484 | cwd: input.cwd, | |
| 1485 | operation: "getReviewThreadComments", | |
| 1486 | }), | |
| 1487 | ), | |
| 1488 | ), | |
| 1489 | ); | |
| 1490 | }, | |
| 1491 | ||
| 1441 | 1492 | listReviewThreadComments: (input) => |
| 1442 | 1493 | Effect.gen(function* () { |
| 1443 | 1494 | const { owner, name } = parseRepositorySelector(input.repository); |
| ... | ... | @@ -1457,25 +1508,6 @@ export const make = Effect.gen(function* () { |
| 1457 | 1508 | query: REVIEW_THREADS_GRAPHQL_QUERY, |
| 1458 | 1509 | decode: decodeReviewThreadsJson, |
| 1459 | 1510 | }); |
| 1460 | const commentPage = ( | |
| 1461 | threadId: string, | |
| 1462 | cursor: string, | |
| 1463 | ): Effect.Effect< | |
| 1464 | { | |
| 1465 | readonly comments: ReadonlyArray<PullRequestThreadComment>; | |
| 1466 | readonly nextCursor: string | null; | |
| 1467 | }, | |
| 1468 | GitHubPullRequestCliError | |
| 1469 | > => | |
| 1470 | graphqlRead({ | |
| 1471 | cwd: input.cwd, | |
| 1472 | host: input.host, | |
| 1473 | operation: "listReviewThreadComments", | |
| 1474 | variables: [["-f", `threadId=${threadId}`], cursorVariable(cursor)], | |
| 1475 | query: REVIEW_THREAD_COMMENTS_GRAPHQL_QUERY, | |
| 1476 | decode: decodeReviewThreadCommentsJson, | |
| 1477 | }); | |
| 1478 | ||
| 1479 | 1511 | const entries: GitHubReviewThreadEntry[] = []; |
| 1480 | 1512 | const avatarsByLogin = new Map<string, string>(); |
| 1481 | 1513 | const commitStats = new Map< |
| ... | ... | @@ -1540,39 +1572,21 @@ export const make = Effect.gen(function* () { |
| 1540 | 1572 | dismissalPage += 1; |
| 1541 | 1573 | } |
| 1542 | 1574 | |
| 1543 | // Only the threads GitHub said were unfinished cost a request; the rest arrived whole | |
| 1544 | // with the page they were listed on. | |
| 1545 | const finished = yield* Effect.forEach( | |
| 1546 | entries, | |
| 1547 | (entry) => | |
| 1548 | Effect.gen(function* () { | |
| 1549 | const comments = [...entry.thread.comments]; | |
| 1550 | let commentCursor = entry.nextCommentCursor; | |
| 1551 | let commentPageCount = 0; | |
| 1552 | while (commentCursor !== null && commentPageCount < REVIEW_THREAD_COMMENT_PAGES) { | |
| 1553 | const read = yield* commentPage(entry.thread.id, commentCursor); | |
| 1554 | comments.push(...read.comments); | |
| 1555 | commentCursor = read.nextCursor; | |
| 1556 | commentPageCount += 1; | |
| 1557 | } | |
| 1558 | return { | |
| 1559 | thread: { ...entry.thread, comments }, | |
| 1560 | commentCount: entry.commentCount, | |
| 1561 | truncated: commentCursor !== null, | |
| 1562 | }; | |
| 1563 | }), | |
| 1564 | { concurrency: REVIEW_THREAD_CONCURRENCY }, | |
| 1565 | ); | |
| 1566 | ||
| 1567 | const reviewThreads = finished.map((entry) => entry.thread); | |
| 1575 | const reviewThreads = entries.map((entry) => ({ | |
| 1576 | ...entry.thread, | |
| 1577 | commentCount: entry.commentCount, | |
| 1578 | ...(entry.nextCommentCursor === null | |
| 1579 | ? {} | |
| 1580 | : { nextCommentsCursor: entry.nextCommentCursor }), | |
| 1581 | })); | |
| 1568 | 1582 | return { |
| 1569 | 1583 | comments: reviewThreadConversation(reviewThreads), |
| 1570 | 1584 | dismissalsByReviewId, |
| 1571 | 1585 | reviewThreads, |
| 1572 | 1586 | // GitHub's own count of each thread, so the number the page shows is the host's even |
| 1573 | 1587 | // where a bound kept some of the words on GitHub. |
| 1574 | commentCount: finished.reduce((total, entry) => total + entry.commentCount, 0), | |
| 1575 | truncated: cursor !== null || finished.some((entry) => entry.truncated), | |
| 1588 | commentCount: entries.reduce((total, entry) => total + entry.commentCount, 0), | |
| 1589 | truncated: cursor !== null || entries.some((entry) => entry.nextCommentCursor !== null), | |
| 1576 | 1590 | reactions, |
| 1577 | 1591 | reactionsById, |
| 1578 | 1592 | reviewers, |
| ... | ... | @@ -1587,34 +1601,14 @@ export const make = Effect.gen(function* () { |
| 1587 | 1601 | if (input.ids.length === 0) { |
| 1588 | 1602 | return Effect.succeed(new Map<string, string>()); |
| 1589 | 1603 | } |
| 1590 | return github | |
| 1591 | .execute({ | |
| 1592 | cwd: input.cwd, | |
| 1593 | args: [ | |
| 1594 | "api", | |
| 1595 | "graphql", | |
| 1596 | "--hostname", | |
| 1597 | input.host, | |
| 1598 | ...input.ids.flatMap((id) => ["-f", `ids[]=${id}`]), | |
| 1599 | "-f", | |
| 1600 | `query=${ACTOR_AVATARS_GRAPHQL_QUERY}`, | |
| 1601 | ], | |
| 1602 | }) | |
| 1603 | .pipe( | |
| 1604 | Effect.flatMap((result) => { | |
| 1605 | const decoded = decodeActorAvatarsJson(result.stdout.trim()); | |
| 1606 | return Result.isSuccess(decoded) | |
| 1607 | ? Effect.succeed(decoded.success) | |
| 1608 | : Effect.fail( | |
| 1609 | new GitHubPullRequestReadError({ | |
| 1610 | command: "gh", | |
| 1611 | cwd: input.cwd, | |
| 1612 | operation: "listActorAvatars", | |
| 1613 | cause: decoded.failure, | |
| 1614 | }), | |
| 1615 | ); | |
| 1616 | }), | |
| 1617 | ); | |
| 1604 | return graphqlRead({ | |
| 1605 | cwd: input.cwd, | |
| 1606 | host: input.host, | |
| 1607 | operation: "listActorAvatars", | |
| 1608 | variables: input.ids.map((id) => ["-f", `ids[]=${id}`]), | |
| 1609 | query: ACTOR_AVATARS_GRAPHQL_QUERY, | |
| 1610 | decode: decodeActorAvatarsJson, | |
| 1611 | }); | |
| 1618 | 1612 | }, |
| 1619 | 1613 | |
| 1620 | 1614 | getRepositoryAccess: (input) => |
| ... | ... | @@ -1651,6 +1645,7 @@ export const make = Effect.gen(function* () { |
| 1651 | 1645 | cwd: input.cwd, |
| 1652 | 1646 | host: input.host, |
| 1653 | 1647 | operation: "getViewerAccess", |
| 1648 | ...(input.allowReserve === true ? { allowReserve: true } : {}), | |
| 1654 | 1649 | variables: [ |
| 1655 | 1650 | ["-f", `owner=${owner}`], |
| 1656 | 1651 | ["-f", `name=${name}`], |
| ... | ... | @@ -1667,6 +1662,7 @@ export const make = Effect.gen(function* () { |
| 1667 | 1662 | cwd: input.cwd, |
| 1668 | 1663 | host: input.host, |
| 1669 | 1664 | operation: "listReviewerCandidates", |
| 1665 | allowReserve: true, | |
| 1670 | 1666 | variables: [ |
| 1671 | 1667 | ["-f", `owner=${owner}`], |
| 1672 | 1668 | ["-f", `name=${name}`], |
apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts+33| ... | ... | @@ -177,6 +177,39 @@ describe("getViewerPermissions", () => { |
| 177 | 177 | ), |
| 178 | 178 | ); |
| 179 | 179 | |
| 180 | it.effect("uses the GraphQL reserve for manual permission checks", () => { | |
| 181 | let viewerAllowReserve: boolean | undefined; | |
| 182 | let comparisonAllowReserve: boolean | undefined; | |
| 183 | return Effect.gen(function* () { | |
| 184 | const provider = yield* make; | |
| 185 | yield* provider.getViewerPermissions({ | |
| 186 | cwd: "/w", | |
| 187 | repository: "acme/web", | |
| 188 | host: "github.com", | |
| 189 | number: 7, | |
| 190 | }); | |
| 191 | ||
| 192 | expect(viewerAllowReserve).toBe(true); | |
| 193 | expect(comparisonAllowReserve).toBe(true); | |
| 194 | }).pipe( | |
| 195 | Effect.provide( | |
| 196 | Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ | |
| 197 | getPullRequestDetail: () => Effect.succeed(openDetail), | |
| 198 | getPullRequestBaseComparison: (input) => | |
| 199 | Effect.sync(() => { | |
| 200 | comparisonAllowReserve = input.allowReserve; | |
| 201 | return { behindBy: 3, viewerCanUpdate: true }; | |
| 202 | }), | |
| 203 | getViewerAccess: (input) => | |
| 204 | Effect.sync(() => { | |
| 205 | viewerAllowReserve = input.allowReserve; | |
| 206 | return { canWrite: true, canUpdate: true, didAuthor: false }; | |
| 207 | }), | |
| 208 | }), | |
| 209 | ), | |
| 210 | ); | |
| 211 | }); | |
| 212 | ||
| 180 | 213 | it.effect("withholds update-branch when the comparison cannot be read", () => |
| 181 | 214 | Effect.gen(function* () { |
| 182 | 215 | const provider = yield* make; |
apps/server/src/pullRequest/GitHubPullRequestProvider.ts+16-7| ... | ... | @@ -9,6 +9,7 @@ import type { |
| 9 | 9 | import * as GitHubPullRequestCli from "./GitHubPullRequestCli.ts"; |
| 10 | 10 | import { |
| 11 | 11 | PullRequestProviderError, |
| 12 | type PullRequestProviderFailure, | |
| 12 | 13 | type ProviderChangeRequestActivity, |
| 13 | 14 | type ProviderChangeRequestDetail, |
| 14 | 15 | type PullRequestProviderApi, |
| ... | ... | @@ -82,12 +83,16 @@ export function gitHubViewerPermissions(access: GitHubViewerAccess): PullRequest |
| 82 | 83 | } |
| 83 | 84 | |
| 84 | 85 | /** The CLI tags that mean the tool itself is unusable, rather than one request failing. */ |
| 85 | function reasonFor( | |
| 86 | export function gitHubProviderFailure( | |
| 86 | 87 | error: GitHubPullRequestCli.GitHubPullRequestCliError, |
| 87 | ): PullRequestProviderError["reason"] { | |
| 88 | if (error._tag === "GitHubCliUnavailableError") return "missing-tool"; | |
| 89 | if (error._tag === "GitHubCliAuthenticationError") return "unauthenticated"; | |
| 90 | return "failed"; | |
| 88 | ): PullRequestProviderFailure { | |
| 89 | if (error._tag === "GitHubCliUnavailableError") return { reason: "missing-tool" }; | |
| 90 | if (error._tag === "GitHubCliAuthenticationError") return { reason: "unauthenticated" }; | |
| 91 | if (error._tag === "GitHubCliRateLimitError") return { reason: "rate-limited" }; | |
| 92 | if (error._tag === "SourceControlRateLimitPausedError") { | |
| 93 | return { reason: "rate-limited", retryAt: error.retryAt }; | |
| 94 | } | |
| 95 | return { reason: "failed" }; | |
| 91 | 96 | } |
| 92 | 97 | |
| 93 | 98 | /** |
| ... | ... | @@ -128,7 +133,7 @@ export const make = Effect.gen(function* () { |
| 128 | 133 | new PullRequestProviderError({ |
| 129 | 134 | provider: "github", |
| 130 | 135 | operation, |
| 131 | reason: reasonFor(error), | |
| 136 | ...gitHubProviderFailure(error), | |
| 132 | 137 | detail: error.detail, |
| 133 | 138 | cause: error, |
| 134 | 139 | }); |
| ... | ... | @@ -355,10 +360,13 @@ export const make = Effect.gen(function* () { |
| 355 | 360 | ), |
| 356 | 361 | ), |
| 357 | 362 | |
| 363 | getReviewThreadComments: (input) => | |
| 364 | cli.getReviewThreadComments(input).pipe(Effect.mapError(fail("getReviewThreadComments"))), | |
| 365 | ||
| 358 | 366 | getViewerPermissions: (input) => |
| 359 | 367 | Effect.all( |
| 360 | 368 | [ |
| 361 | cli.getViewerAccess(input), | |
| 369 | cli.getViewerAccess({ ...input, allowReserve: true }), | |
| 362 | 370 | // Whether this viewer may update the branch is only on the comparison, and the |
| 363 | 371 | // comparison only resolves through the head ref the detail carries. A failure here |
| 364 | 372 | // withholds that one action rather than the whole answer, the way the detail path |
| ... | ... | @@ -371,6 +379,7 @@ export const make = Effect.gen(function* () { |
| 371 | 379 | .getPullRequestBaseComparison({ |
| 372 | 380 | ...input, |
| 373 | 381 | headRef: `${pullRequest.headRepositoryOwner}:${pullRequest.headBranch}`, |
| 382 | allowReserve: true, | |
| 374 | 383 | }) |
| 375 | 384 | .pipe(Effect.map((comparison) => comparison.viewerCanUpdate === true)), |
| 376 | 385 | ), |
apps/server/src/pullRequest/GitLabPullRequestProvider.ts+8-6| ... | ... | @@ -8,6 +8,7 @@ import type { |
| 8 | 8 | import * as GitLabPullRequestCli from "./GitLabPullRequestCli.ts"; |
| 9 | 9 | import { |
| 10 | 10 | PullRequestProviderError, |
| 11 | type PullRequestProviderFailure, | |
| 11 | 12 | type ProviderChangeRequestActivity, |
| 12 | 13 | type ProviderChangeRequestDetail, |
| 13 | 14 | type PullRequestProviderApi, |
| ... | ... | @@ -90,12 +91,13 @@ export function gitLabViewerPermissions(input: { |
| 90 | 91 | } |
| 91 | 92 | |
| 92 | 93 | /** The CLI tags that mean the tool itself is unusable, rather than one request failing. */ |
| 93 | function reasonFor( | |
| 94 | export function gitLabProviderFailure( | |
| 94 | 95 | error: GitLabPullRequestCli.GitLabPullRequestCliError, |
| 95 | ): PullRequestProviderError["reason"] { | |
| 96 | if (error._tag === "GitLabCliUnavailableError") return "missing-tool"; | |
| 97 | if (error._tag === "GitLabCliAuthenticationError") return "unauthenticated"; | |
| 98 | return "failed"; | |
| 96 | ): PullRequestProviderFailure { | |
| 97 | if (error._tag === "GitLabCliUnavailableError") return { reason: "missing-tool" }; | |
| 98 | if (error._tag === "GitLabCliAuthenticationError") return { reason: "unauthenticated" }; | |
| 99 | if (error._tag === "GitLabCliRateLimitError") return { reason: "rate-limited" }; | |
| 100 | return { reason: "failed" }; | |
| 99 | 101 | } |
| 100 | 102 | |
| 101 | 103 | export const make = Effect.gen(function* () { |
| ... | ... | @@ -105,7 +107,7 @@ export const make = Effect.gen(function* () { |
| 105 | 107 | new PullRequestProviderError({ |
| 106 | 108 | provider: "gitlab", |
| 107 | 109 | operation, |
| 108 | reason: reasonFor(error), | |
| 110 | ...gitLabProviderFailure(error), | |
| 109 | 111 | detail: error.detail, |
| 110 | 112 | cause: error, |
| 111 | 113 | }); |
apps/server/src/pullRequest/PullRequestProvider.ts+19-2| ... | ... | @@ -22,6 +22,7 @@ import type { |
| 22 | 22 | PullRequestReviewCommentDraft, |
| 23 | 23 | PullRequestReviewDecision, |
| 24 | 24 | PullRequestReviewThread, |
| 25 | PullRequestThreadCommentsResult, | |
| 25 | 26 | PullRequestReviewVerdict, |
| 26 | 27 | PullRequestReviewerCandidateList, |
| 27 | 28 | PullRequestReviewerKind, |
| ... | ... | @@ -37,15 +38,17 @@ import { SourceControlProviderKind as SourceControlProviderKindSchema } from "@t |
| 37 | 38 | * without knowing which CLI or API produced it. |
| 38 | 39 | * |
| 39 | 40 | * `reason` is the part the service acts on: a missing or unauthenticated tool disables the |
| 40 | * provider for the whole workspace, while anything else is specific to the request. | |
| 41 | * provider for the whole workspace, a rate limit pauses its host, and anything else is specific | |
| 42 | * to the request. | |
| 41 | 43 | */ |
| 42 | 44 | export class PullRequestProviderError extends Schema.TaggedErrorClass<PullRequestProviderError>()( |
| 43 | 45 | "PullRequestProviderError", |
| 44 | 46 | { |
| 45 | 47 | provider: SourceControlProviderKindSchema, |
| 46 | 48 | operation: Schema.String, |
| 47 | reason: Schema.Literals(["missing-tool", "unauthenticated", "failed"]), | |
| 49 | reason: Schema.Literals(["missing-tool", "unauthenticated", "rate-limited", "failed"]), | |
| 48 | 50 | detail: Schema.String, |
| 51 | retryAt: Schema.optional(Schema.Number), | |
| 49 | 52 | cause: Schema.optional(Schema.Defect()), |
| 50 | 53 | }, |
| 51 | 54 | ) { |
| ... | ... | @@ -54,6 +57,11 @@ export class PullRequestProviderError extends Schema.TaggedErrorClass<PullReques |
| 54 | 57 | } |
| 55 | 58 | } |
| 56 | 59 | |
| 60 | export interface PullRequestProviderFailure { | |
| 61 | readonly reason: PullRequestProviderError["reason"]; | |
| 62 | readonly retryAt?: number | undefined; | |
| 63 | } | |
| 64 | ||
| 57 | 65 | /** A change request as the provider sees it, before the service attaches project context. */ |
| 58 | 66 | export interface ProviderChangeRequest { |
| 59 | 67 | readonly number: number; |
| ... | ... | @@ -297,6 +305,15 @@ export interface PullRequestProviderApi { |
| 297 | 305 | input: ProviderRepositoryRef & { readonly number: number }, |
| 298 | 306 | ) => Effect.Effect<ProviderChangeRequestActivity, PullRequestProviderError>; |
| 299 | 307 | |
| 308 | /** One explicit page after a reader asks to continue an unfinished review thread. */ | |
| 309 | readonly getReviewThreadComments?: ( | |
| 310 | input: ProviderRepositoryRef & { | |
| 311 | readonly number: number; | |
| 312 | readonly threadId: string; | |
| 313 | readonly cursor: string; | |
| 314 | }, | |
| 315 | ) => Effect.Effect<PullRequestThreadCommentsResult, PullRequestProviderError>; | |
| 316 | ||
| 300 | 317 | /** |
| 301 | 318 | * The same answer `getChangeRequest` carries, on its own. Asked before anything is written, so |
| 302 | 319 | * a request that reached the server without going past the page is refused by what the host |
apps/server/src/pullRequest/PullRequestProviderRateLimit.test.ts created+69| ... | ... | @@ -0,0 +1,69 @@ |
| 1 | import { assert, it } from "@effect/vitest"; | |
| 2 | ||
| 3 | import * as AzureDevOpsCli from "../sourceControl/AzureDevOpsCli.ts"; | |
| 4 | import * as BitbucketApi from "../sourceControl/BitbucketApi.ts"; | |
| 5 | import * as GitHubCli from "../sourceControl/GitHubCli.ts"; | |
| 6 | import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; | |
| 7 | import * as GitLabCli from "../sourceControl/GitLabCli.ts"; | |
| 8 | import { azureDevOpsProviderFailure } from "./AzureDevOpsPullRequestProvider.ts"; | |
| 9 | import { bitbucketProviderFailure } from "./BitbucketPullRequestProvider.ts"; | |
| 10 | import { gitHubProviderFailure } from "./GitHubPullRequestProvider.ts"; | |
| 11 | import { gitLabProviderFailure } from "./GitLabPullRequestProvider.ts"; | |
| 12 | ||
| 13 | const cause = new Error("redacted provider failure"); | |
| 14 | ||
| 15 | it("classifies rate limits from every pull-request provider", () => { | |
| 16 | assert.deepStrictEqual( | |
| 17 | gitHubProviderFailure( | |
| 18 | new GitHubCli.GitHubCliRateLimitError({ command: "gh", cwd: "/repo", cause }), | |
| 19 | ), | |
| 20 | { reason: "rate-limited" }, | |
| 21 | ); | |
| 22 | assert.deepStrictEqual( | |
| 23 | gitLabProviderFailure( | |
| 24 | new GitLabCli.GitLabCliRateLimitError({ | |
| 25 | operation: "execute", | |
| 26 | command: "glab", | |
| 27 | cwd: "/repo", | |
| 28 | cause, | |
| 29 | }), | |
| 30 | ), | |
| 31 | { reason: "rate-limited" }, | |
| 32 | ); | |
| 33 | assert.deepStrictEqual( | |
| 34 | azureDevOpsProviderFailure( | |
| 35 | new AzureDevOpsCli.AzureDevOpsCliRateLimitError({ | |
| 36 | operation: "execute", | |
| 37 | command: "az", | |
| 38 | cwd: "/repo", | |
| 39 | argumentCount: 1, | |
| 40 | cause, | |
| 41 | }), | |
| 42 | ), | |
| 43 | { reason: "rate-limited" }, | |
| 44 | ); | |
| 45 | assert.deepStrictEqual( | |
| 46 | bitbucketProviderFailure( | |
| 47 | new BitbucketApi.BitbucketResponseError({ | |
| 48 | operation: "request", | |
| 49 | status: 429, | |
| 50 | responseBodyLength: 0, | |
| 51 | retryAt: 120_000, | |
| 52 | }), | |
| 53 | ), | |
| 54 | { reason: "rate-limited", retryAt: 120_000 }, | |
| 55 | ); | |
| 56 | }); | |
| 57 | ||
| 58 | it("keeps GitHub's exact retry time", () => { | |
| 59 | assert.deepStrictEqual( | |
| 60 | gitHubProviderFailure( | |
| 61 | new SourceControlRateLimit.SourceControlRateLimitPausedError({ | |
| 62 | provider: "github", | |
| 63 | host: "github.com", | |
| 64 | retryAt: 1_786_802_400_000, | |
| 65 | }), | |
| 66 | ), | |
| 67 | { reason: "rate-limited", retryAt: 1_786_802_400_000 }, | |
| 68 | ); | |
| 69 | }); |
apps/server/src/pullRequest/PullRequestProviderRegistry.ts+7-1| ... | ... | @@ -6,6 +6,7 @@ import type { SourceControlProviderKind } from "@t3tools/contracts"; |
| 6 | 6 | import * as AzureDevOpsCli from "../sourceControl/AzureDevOpsCli.ts"; |
| 7 | 7 | import * as BitbucketApi from "../sourceControl/BitbucketApi.ts"; |
| 8 | 8 | import * as GitHubCli from "../sourceControl/GitHubCli.ts"; |
| 9 | import * as GitHubGraphQlBudget from "../sourceControl/githubGraphQlBudget.ts"; | |
| 9 | 10 | import * as GitLabCli from "../sourceControl/GitLabCli.ts"; |
| 10 | 11 | import * as AzureDevOpsPullRequestCli from "./AzureDevOpsPullRequestCli.ts"; |
| 11 | 12 | import * as AzureDevOpsPullRequestProvider from "./AzureDevOpsPullRequestProvider.ts"; |
| ... | ... | @@ -52,7 +53,12 @@ export const make = Effect.map( |
| 52 | 53 | ); |
| 53 | 54 | |
| 54 | 55 | export const layer = Layer.effect(PullRequestProviderRegistry, make).pipe( |
| 55 | Layer.provide(GitHubPullRequestCli.layer.pipe(Layer.provide(GitHubCli.layer))), | |
| 56 | Layer.provide( | |
| 57 | GitHubPullRequestCli.layer.pipe( | |
| 58 | Layer.provide(GitHubCli.layer), | |
| 59 | Layer.provide(GitHubGraphQlBudget.layer), | |
| 60 | ), | |
| 61 | ), | |
| 56 | 62 | Layer.provide(GitLabPullRequestCli.layer.pipe(Layer.provide(GitLabCli.layer))), |
| 57 | 63 | Layer.provide(BitbucketPullRequestApi.layer.pipe(Layer.provide(BitbucketApi.layer))), |
| 58 | 64 | Layer.provide(AzureDevOpsPullRequestCli.layer.pipe(Layer.provide(AzureDevOpsCli.layer))), |
apps/server/src/pullRequest/PullRequestService.test.ts+93| ... | ... | @@ -11,6 +11,7 @@ import type { |
| 11 | 11 | |
| 12 | 12 | import * as ProjectionSnapshotQuery from "../orchestration/Services/ProjectionSnapshotQuery.ts"; |
| 13 | 13 | import * as SourceControlProviderRegistry from "../sourceControl/SourceControlProviderRegistry.ts"; |
| 14 | import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; | |
| 14 | 15 | import { |
| 15 | 16 | PullRequestProviderError, |
| 16 | 17 | type ProviderChangeRequest, |
| ... | ... | @@ -169,6 +170,7 @@ function makeService(input: { |
| 169 | 170 | updatedAt: "2026-07-01T00:00:00Z", |
| 170 | 171 | }), |
| 171 | 172 | }), |
| 173 | SourceControlRateLimit.layer, | |
| 172 | 174 | ), |
| 173 | 175 | ), |
| 174 | 176 | ); |
| ... | ... | @@ -1339,6 +1341,97 @@ it.effect("reports repositories on a host that could not be read", () => |
| 1339 | 1341 | }), |
| 1340 | 1342 | ); |
| 1341 | 1343 | |
| 1344 | it.effect("stops new reads after a rate limit while leaving manual actions available", () => | |
| 1345 | Effect.gen(function* () { | |
| 1346 | let listCalls = 0; | |
| 1347 | let actionCalls = 0; | |
| 1348 | const service = yield* makeService({ | |
| 1349 | projects: [ | |
| 1350 | project({ id: "p1", title: "cloud", workspaceRoot: "/cloud", repository: "acme/web" }), | |
| 1351 | ], | |
| 1352 | providers: [ | |
| 1353 | fakeProvider("github", { | |
| 1354 | listChangeRequests: () => | |
| 1355 | Effect.sync(() => { | |
| 1356 | listCalls += 1; | |
| 1357 | }).pipe( | |
| 1358 | Effect.andThen( | |
| 1359 | Effect.fail( | |
| 1360 | new PullRequestProviderError({ | |
| 1361 | provider: "github", | |
| 1362 | operation: "listChangeRequests", | |
| 1363 | reason: "rate-limited", | |
| 1364 | detail: "GitHub API rate limit exceeded.", | |
| 1365 | }), | |
| 1366 | ), | |
| 1367 | ), | |
| 1368 | ), | |
| 1369 | runAction: () => | |
| 1370 | Effect.sync(() => { | |
| 1371 | actionCalls += 1; | |
| 1372 | }), | |
| 1373 | }), | |
| 1374 | ], | |
| 1375 | }); | |
| 1376 | ||
| 1377 | const first = yield* service.list({ state: "open", involvement: "all" }); | |
| 1378 | const paused = yield* service.list({ state: "open", involvement: "authored" }); | |
| 1379 | yield* service.runAction({ | |
| 1380 | projectId: "p1" as ProjectId, | |
| 1381 | repository: "acme/web", | |
| 1382 | number: 1, | |
| 1383 | action: "close", | |
| 1384 | }); | |
| 1385 | ||
| 1386 | assert.strictEqual(listCalls, 1); | |
| 1387 | assert.strictEqual(actionCalls, 1); | |
| 1388 | assert.lengthOf(first.errors, 1); | |
| 1389 | assert.lengthOf(paused.errors, 1); | |
| 1390 | }), | |
| 1391 | ); | |
| 1392 | ||
| 1393 | it.effect("uses a manual rate limit to pause later reads", () => | |
| 1394 | Effect.gen(function* () { | |
| 1395 | let listCalls = 0; | |
| 1396 | const service = yield* makeService({ | |
| 1397 | projects: [ | |
| 1398 | project({ id: "p1", title: "cloud", workspaceRoot: "/cloud", repository: "acme/web" }), | |
| 1399 | ], | |
| 1400 | providers: [ | |
| 1401 | fakeProvider("github", { | |
| 1402 | listChangeRequests: () => | |
| 1403 | Effect.sync(() => { | |
| 1404 | listCalls += 1; | |
| 1405 | return { items: [], truncated: false, continues: true }; | |
| 1406 | }), | |
| 1407 | runAction: () => | |
| 1408 | Effect.fail( | |
| 1409 | new PullRequestProviderError({ | |
| 1410 | provider: "github", | |
| 1411 | operation: "runAction", | |
| 1412 | reason: "rate-limited", | |
| 1413 | detail: "GitHub API rate limit exceeded.", | |
| 1414 | }), | |
| 1415 | ), | |
| 1416 | }), | |
| 1417 | ], | |
| 1418 | }); | |
| 1419 | ||
| 1420 | yield* Effect.flip( | |
| 1421 | service.runAction({ | |
| 1422 | projectId: "p1" as ProjectId, | |
| 1423 | repository: "acme/web", | |
| 1424 | number: 1, | |
| 1425 | action: "close", | |
| 1426 | }), | |
| 1427 | ); | |
| 1428 | const error = yield* Effect.flip(service.list({ state: "open", involvement: "all" })); | |
| 1429 | ||
| 1430 | assert.strictEqual(listCalls, 0); | |
| 1431 | assert.strictEqual(error._tag, "PullRequestOperationError"); | |
| 1432 | }), | |
| 1433 | ); | |
| 1434 | ||
| 1342 | 1435 | it.effect("flags a review request for the viewer but not on their own change request", () => |
| 1343 | 1436 | Effect.gen(function* () { |
| 1344 | 1437 | const service = yield* makeService({ |
apps/server/src/pullRequest/PullRequestService.ts+137-2| ... | ... | @@ -40,6 +40,8 @@ import { |
| 40 | 40 | type PullRequestSubmitReviewInput, |
| 41 | 41 | type PullRequestThreadReplyInput, |
| 42 | 42 | type PullRequestThreadResolutionInput, |
| 43 | type PullRequestThreadCommentsInput, | |
| 44 | type PullRequestThreadCommentsResult, | |
| 43 | 45 | type PullRequestUpdateInput, |
| 44 | 46 | type SourceControlProviderInfo, |
| 45 | 47 | type SourceControlProviderKind, |
| ... | ... | @@ -48,11 +50,12 @@ import { detectSourceControlProviderFromRemoteUrl } from "@t3tools/shared/source |
| 48 | 50 | |
| 49 | 51 | import * as ProjectionSnapshotQuery from "../orchestration/Services/ProjectionSnapshotQuery.ts"; |
| 50 | 52 | import * as SourceControlProviderRegistry from "../sourceControl/SourceControlProviderRegistry.ts"; |
| 53 | import * as SourceControlRateLimit from "../sourceControl/SourceControlRateLimit.ts"; | |
| 51 | 54 | import { |
| 52 | 55 | type ProviderChangeRequest, |
| 53 | 56 | type ProviderListCursor, |
| 54 | 57 | type PullRequestProviderApi, |
| 55 | type PullRequestProviderError, | |
| 58 | PullRequestProviderError, | |
| 56 | 59 | } from "./PullRequestProvider.ts"; |
| 57 | 60 | import { PullRequestProviderRegistry } from "./PullRequestProviderRegistry.ts"; |
| 58 | 61 | |
| ... | ... | @@ -132,6 +135,9 @@ export class PullRequestService extends Context.Service< |
| 132 | 135 | readonly activity: ( |
| 133 | 136 | input: PullRequestRef, |
| 134 | 137 | ) => Effect.Effect<PullRequestActivity, PullRequestError>; |
| 138 | readonly threadComments: ( | |
| 139 | input: PullRequestThreadCommentsInput, | |
| 140 | ) => Effect.Effect<PullRequestThreadCommentsResult, PullRequestError>; | |
| 135 | 141 | readonly diff: ( |
| 136 | 142 | input: PullRequestDiffInput, |
| 137 | 143 | ) => Effect.Effect<PullRequestDiffResult, PullRequestError>; |
| ... | ... | @@ -366,6 +372,103 @@ function toPullRequestError( |
| 366 | 372 | : new PullRequestOperationError({ operation, detail: error.detail, cause: error }); |
| 367 | 373 | } |
| 368 | 374 | |
| 375 | function withRateLimitBackoff( | |
| 376 | api: PullRequestProviderApi, | |
| 377 | host: string, | |
| 378 | limits: SourceControlRateLimit.SourceControlRateLimit["Service"], | |
| 379 | ): PullRequestProviderApi { | |
| 380 | const key = { provider: api.kind, host }; | |
| 381 | const protect = <A>( | |
| 382 | operation: string, | |
| 383 | effect: Effect.Effect<A, PullRequestProviderError>, | |
| 384 | allowPaused: boolean, | |
| 385 | ) => | |
| 386 | limits.check(key, allowPaused ? { allowPaused: true } : undefined).pipe( | |
| 387 | Effect.mapError( | |
| 388 | (error) => | |
| 389 | new PullRequestProviderError({ | |
| 390 | provider: api.kind, | |
| 391 | operation, | |
| 392 | reason: "rate-limited", | |
| 393 | detail: error.detail, | |
| 394 | retryAt: error.retryAt, | |
| 395 | cause: error, | |
| 396 | }), | |
| 397 | ), | |
| 398 | Effect.flatMap((lease) => | |
| 399 | effect.pipe( | |
| 400 | Effect.tap(() => limits.recordSuccess({ ...key, lease })), | |
| 401 | Effect.tapError((error) => | |
| 402 | error.reason === "rate-limited" | |
| 403 | ? limits.recordRateLimit({ | |
| 404 | ...key, | |
| 405 | lease, | |
| 406 | ...(error.retryAt === undefined ? {} : { retryAt: error.retryAt }), | |
| 407 | }) | |
| 408 | : Effect.void, | |
| 409 | ), | |
| 410 | ), | |
| 411 | ), | |
| 412 | ); | |
| 413 | const wrap = | |
| 414 | <Args extends ReadonlyArray<unknown>, A>( | |
| 415 | operation: string, | |
| 416 | call: (...args: Args) => Effect.Effect<A, PullRequestProviderError>, | |
| 417 | allowPaused = false, | |
| 418 | ) => | |
| 419 | (...args: Args) => | |
| 420 | protect(operation, call(...args), allowPaused); | |
| 421 | const interactive = <Args extends ReadonlyArray<unknown>, A>( | |
| 422 | operation: string, | |
| 423 | call: (...args: Args) => Effect.Effect<A, PullRequestProviderError>, | |
| 424 | ) => wrap(operation, call, true); | |
| 425 | ||
| 426 | return { | |
| 427 | kind: api.kind, | |
| 428 | capabilities: api.capabilities, | |
| 429 | getViewer: wrap("getViewer", api.getViewer), | |
| 430 | listChangeRequests: wrap("listChangeRequests", api.listChangeRequests), | |
| 431 | ...(api.listChangeRequestsAcross === undefined | |
| 432 | ? {} | |
| 433 | : { | |
| 434 | listChangeRequestsAcross: wrap("listChangeRequestsAcross", api.listChangeRequestsAcross), | |
| 435 | }), | |
| 436 | ...(api.listChangeRequestStats === undefined | |
| 437 | ? {} | |
| 438 | : { | |
| 439 | listChangeRequestStats: wrap("listChangeRequestStats", api.listChangeRequestStats), | |
| 440 | }), | |
| 441 | getChangeRequest: wrap("getChangeRequest", api.getChangeRequest), | |
| 442 | getChangeRequestActivity: wrap("getChangeRequestActivity", api.getChangeRequestActivity), | |
| 443 | ...(api.getReviewThreadComments === undefined | |
| 444 | ? {} | |
| 445 | : { | |
| 446 | getReviewThreadComments: wrap("getReviewThreadComments", api.getReviewThreadComments), | |
| 447 | }), | |
| 448 | getViewerPermissions: interactive("getViewerPermissions", api.getViewerPermissions), | |
| 449 | getDiff: wrap("getDiff", api.getDiff), | |
| 450 | ...(api.getDiffFileContents === undefined | |
| 451 | ? {} | |
| 452 | : { getDiffFileContents: wrap("getDiffFileContents", api.getDiffFileContents) }), | |
| 453 | runAction: interactive("runAction", api.runAction), | |
| 454 | ...(api.updateChangeRequest === undefined | |
| 455 | ? {} | |
| 456 | : { | |
| 457 | updateChangeRequest: interactive("updateChangeRequest", api.updateChangeRequest), | |
| 458 | }), | |
| 459 | comment: interactive("comment", api.comment), | |
| 460 | ...(api.updateComment === undefined | |
| 461 | ? {} | |
| 462 | : { updateComment: interactive("updateComment", api.updateComment) }), | |
| 463 | submitReview: interactive("submitReview", api.submitReview), | |
| 464 | listReviewerCandidates: interactive("listReviewerCandidates", api.listReviewerCandidates), | |
| 465 | setReviewerRequest: interactive("setReviewerRequest", api.setReviewerRequest), | |
| 466 | replyToThread: interactive("replyToThread", api.replyToThread), | |
| 467 | setReaction: interactive("setReaction", api.setReaction), | |
| 468 | setThreadResolution: interactive("setThreadResolution", api.setThreadResolution), | |
| 469 | }; | |
| 470 | } | |
| 471 | ||
| 369 | 472 | /** |
| 370 | 473 | * The provider-native repository selector. `displayName` is the full path below the host, which |
| 371 | 474 | * is what nested GitLab groups need; owner/name is the two-segment fallback for identities |
| ... | ... | @@ -394,6 +497,7 @@ export const make = Effect.gen(function* () { |
| 394 | 497 | const registry = yield* PullRequestProviderRegistry; |
| 395 | 498 | const projections = yield* ProjectionSnapshotQuery.ProjectionSnapshotQuery; |
| 396 | 499 | const sourceControlProviders = yield* SourceControlProviderRegistry.SourceControlProviderRegistry; |
| 500 | const rateLimits = yield* SourceControlRateLimit.SourceControlRateLimit; | |
| 397 | 501 | |
| 398 | 502 | const refineUnknownProjectKinds = ( |
| 399 | 503 | projects: ReadonlyArray<OrchestrationProjectShell>, |
| ... | ... | @@ -511,7 +615,12 @@ export const make = Effect.gen(function* () { |
| 511 | 615 | else counted.projectCount += 1; |
| 512 | 616 | continue; |
| 513 | 617 | } |
| 514 | supported.push({ project, api, repository, host }); | |
| 618 | supported.push({ | |
| 619 | project, | |
| 620 | api: withRateLimitBackoff(api, host, rateLimits), | |
| 621 | repository, | |
| 622 | host, | |
| 623 | }); | |
| 515 | 624 | } |
| 516 | 625 | return { supported, unimplemented, viewerRoots }; |
| 517 | 626 | }), |
| ... | ... | @@ -1116,6 +1225,31 @@ export const make = Effect.gen(function* () { |
| 1116 | 1225 | ), |
| 1117 | 1226 | ); |
| 1118 | 1227 | |
| 1228 | const threadComments: PullRequestService["Service"]["threadComments"] = (input) => | |
| 1229 | requireProject(input).pipe( | |
| 1230 | Effect.flatMap( | |
| 1231 | (project): Effect.Effect<PullRequestThreadCommentsResult, PullRequestError> => { | |
| 1232 | const read = project.api.getReviewThreadComments; | |
| 1233 | if (read === undefined) { | |
| 1234 | return Effect.fail( | |
| 1235 | new PullRequestOperationError({ | |
| 1236 | operation: "threadComments", | |
| 1237 | detail: "This host does not page review thread comments.", | |
| 1238 | }), | |
| 1239 | ); | |
| 1240 | } | |
| 1241 | return read({ | |
| 1242 | cwd: project.project.workspaceRoot, | |
| 1243 | repository: project.repository, | |
| 1244 | host: project.host, | |
| 1245 | number: input.number, | |
| 1246 | threadId: input.threadId, | |
| 1247 | cursor: input.cursor, | |
| 1248 | }).pipe(Effect.mapError(toPullRequestError("threadComments"))); | |
| 1249 | }, | |
| 1250 | ), | |
| 1251 | ); | |
| 1252 | ||
| 1119 | 1253 | const diffUncached: PullRequestService["Service"]["diff"] = (input) => |
| 1120 | 1254 | requireProject(input).pipe( |
| 1121 | 1255 | Effect.flatMap((project) => |
| ... | ... | @@ -1993,6 +2127,7 @@ export const make = Effect.gen(function* () { |
| 1993 | 2127 | listStats, |
| 1994 | 2128 | detail, |
| 1995 | 2129 | activity, |
| 2130 | threadComments, | |
| 1996 | 2131 | diff, |
| 1997 | 2132 | diffFileContents, |
| 1998 | 2133 | runAction: invalidatedByMutation(runAction), |
apps/server/src/pullRequest/gitHubPullRequestJson.test.ts+16| ... | ... | @@ -604,7 +604,9 @@ describe("review thread decoding", () => { |
| 604 | 604 | decodeReviewThreadCommentsJson( |
| 605 | 605 | JSON.stringify({ |
| 606 | 606 | data: { |
| 607 | repository: { pullRequest: { id: "PR_1" } }, | |
| 607 | 608 | node: { |
| 609 | pullRequest: { id: "PR_1" }, | |
| 608 | 610 | comments: { |
| 609 | 611 | pageInfo: { hasNextPage: false, endCursor: "Y3Vyc29yOjk" }, |
| 610 | 612 | nodes: [{ id: "t9", body: "last", createdAt: "2026-07-01T00:00:00Z" }], |
| ... | ... | @@ -623,7 +625,9 @@ describe("reaction decoding", () => { |
| 623 | 625 | const commentWithGroups = (reactionGroups: ReadonlyArray<Record<string, unknown>>) => |
| 624 | 626 | JSON.stringify({ |
| 625 | 627 | data: { |
| 628 | repository: { pullRequest: { id: "PR_1" } }, | |
| 626 | 629 | node: { |
| 630 | pullRequest: { id: "PR_1" }, | |
| 627 | 631 | comments: { |
| 628 | 632 | pageInfo: { hasNextPage: false, endCursor: null }, |
| 629 | 633 | nodes: [{ id: "t1", body: "nice", createdAt: "2026-07-01T00:00:00Z", reactionGroups }], |
| ... | ... | @@ -669,7 +673,9 @@ describe("reaction decoding", () => { |
| 669 | 673 | JSON.stringify({ |
| 670 | 674 | data: { |
| 671 | 675 | viewer: { login: "Bilal" }, |
| 676 | repository: { pullRequest: { id: "PR_1" } }, | |
| 672 | 677 | node: { |
| 678 | pullRequest: { id: "PR_1" }, | |
| 673 | 679 | comments: { |
| 674 | 680 | pageInfo: { hasNextPage: false, endCursor: null }, |
| 675 | 681 | nodes: [ |
| ... | ... | @@ -1009,6 +1015,16 @@ describe("decodePullRequestNodeIdJson", () => { |
| 1009 | 1015 | }); |
| 1010 | 1016 | |
| 1011 | 1017 | describe("REVIEW_THREADS_GRAPHQL_QUERY", () => { |
| 1018 | it("caps the initial query after the 104-point rate-limit regression", () => { | |
| 1019 | const match = REVIEW_THREADS_GRAPHQL_QUERY.match( | |
| 1020 | /reviewThreads\(first: (\d+)[\s\S]*?comments\(first: (\d+)\)/u, | |
| 1021 | ); | |
| 1022 | ||
| 1023 | expect(match).not.toBeNull(); | |
| 1024 | if (match === null) throw new Error("expected review-thread connections"); | |
| 1025 | expect(Number(match[1]) * Number(match[2])).toBeLessThanOrEqual(1_000); | |
| 1026 | }); | |
| 1027 | ||
| 1012 | 1028 | it("asks for reactionGroups on the pull request itself, its comments, its reviews and each thread's comments", () => { |
| 1013 | 1029 | expect(REVIEW_THREADS_GRAPHQL_QUERY.match(/reactionGroups/g)).toHaveLength(4); |
| 1014 | 1030 | // The reviews connection is new: only reactions were ever wanted off it. |
apps/server/src/pullRequest/gitHubPullRequestJson.ts+22-3| ... | ... | @@ -661,6 +661,10 @@ export function pullRequestSearchGraphQlQuery(rows: number): string { |
| 661 | 661 | * null for the first page and the last page's `endCursor` after that, so a pull request with |
| 662 | 662 | * more threads than one page holds is walked rather than cut off at the first fifty. |
| 663 | 663 | * |
| 664 | * Only ten comments ride with each thread. A hundred threads times a hundred comments made | |
| 665 | * GitHub reserve 10,000 nested rows and charge 104 points; unfinished threads are paged from | |
| 666 | * their own cursor below. | |
| 667 | * | |
| 664 | 668 | * Reviewers come from here rather than from `gh pr view --json reviewRequests` for two reasons: |
| 665 | 669 | * that field holds only requests still outstanding, so anyone who has already reviewed drops off |
| 666 | 670 | * it, and neither it nor any other `gh` JSON field carries an avatar. A reviewer can be a person |
| ... | ... | @@ -689,7 +693,7 @@ export const REVIEW_THREADS_GRAPHQL_QUERY = `query($owner: String!, $name: Strin |
| 689 | 693 | path |
| 690 | 694 | line |
| 691 | 695 | diffSide |
| 692 | comments(first: ${GRAPHQL_PAGE_SIZE}) { | |
| 696 | comments(first: 10) { | |
| 693 | 697 | totalCount |
| 694 | 698 | pageInfo { hasNextPage endCursor } |
| 695 | 699 | nodes { id author { login avatarUrl } body createdAt url ${REACTION_GROUPS_FIELDS} } |
| ... | ... | @@ -740,10 +744,12 @@ export const REVIEW_THREADS_GRAPHQL_QUERY = `query($owner: String!, $name: Strin |
| 740 | 744 | * from the inner node itself, so a thread longer than a page is followed on its own — a request |
| 741 | 745 | * GitHub makes necessary, and one no ordinary pull request ever provokes. |
| 742 | 746 | */ |
| 743 | export const REVIEW_THREAD_COMMENTS_GRAPHQL_QUERY = `query($threadId: ID!, $cursor: String) { | |
| 747 | export const REVIEW_THREAD_COMMENTS_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!, $threadId: ID!, $cursor: String) { | |
| 744 | 748 | viewer { login } |
| 749 | repository(owner: $owner, name: $name) { pullRequest(number: $number) { id } } | |
| 745 | 750 | node(id: $threadId) { |
| 746 | 751 | ... on PullRequestReviewThread { |
| 752 | pullRequest { id } | |
| 747 | 753 | comments(first: ${GRAPHQL_PAGE_SIZE}, after: $cursor) { |
| 748 | 754 | pageInfo { hasNextPage endCursor } |
| 749 | 755 | nodes { id author { login avatarUrl } body createdAt url ${REACTION_GROUPS_FIELDS} } |
| ... | ... | @@ -757,8 +763,16 @@ const RawReviewThreadCommentsSchema = Schema.Struct({ |
| 757 | 763 | viewer: Schema.optional( |
| 758 | 764 | Schema.NullOr(Schema.Struct({ login: Schema.optional(Schema.NullOr(Schema.String)) })), |
| 759 | 765 | ), |
| 766 | repository: Schema.NullOr( | |
| 767 | Schema.Struct({ pullRequest: Schema.NullOr(Schema.Struct({ id: Schema.String })) }), | |
| 768 | ), | |
| 760 | 769 | /** Null for an id that names nothing the viewer can read, which is not a thread to page. */ |
| 761 | node: Schema.NullOr(Schema.Struct({ comments: Schema.optional(RawThreadCommentsSchema) })), | |
| 770 | node: Schema.NullOr( | |
| 771 | Schema.Struct({ | |
| 772 | pullRequest: Schema.optional(Schema.Struct({ id: Schema.String })), | |
| 773 | comments: Schema.optional(RawThreadCommentsSchema), | |
| 774 | }), | |
| 775 | ), | |
| 762 | 776 | }), |
| 763 | 777 | }); |
| 764 | 778 | |
| ... | ... | @@ -1820,6 +1834,7 @@ export function decodeReviewThreadsJson( |
| 1820 | 1834 | /** The rest of one thread's comments, in the shape the first page already delivered them. */ |
| 1821 | 1835 | export function decodeReviewThreadCommentsJson(raw: string): Result.Result< |
| 1822 | 1836 | { |
| 1837 | readonly belongsToPullRequest: boolean; | |
| 1823 | 1838 | readonly comments: ReadonlyArray<PullRequestThreadComment>; |
| 1824 | 1839 | readonly nextCursor: string | null; |
| 1825 | 1840 | }, |
| ... | ... | @@ -1832,6 +1847,10 @@ export function decodeReviewThreadCommentsJson(raw: string): Result.Result< |
| 1832 | 1847 | const viewer = trimmed(decoded.success.data.viewer?.login); |
| 1833 | 1848 | const comments = decoded.success.data.node?.comments; |
| 1834 | 1849 | return Result.succeed({ |
| 1850 | belongsToPullRequest: | |
| 1851 | decoded.success.data.repository?.pullRequest?.id !== undefined && | |
| 1852 | decoded.success.data.repository?.pullRequest?.id === | |
| 1853 | decoded.success.data.node?.pullRequest?.id, | |
| 1835 | 1854 | comments: (comments?.nodes ?? []).map((comment) => ({ |
| 1836 | 1855 | id: comment.id, |
| 1837 | 1856 | author: toActor(comment.author), |
apps/server/src/server.ts+2| ... | ... | @@ -78,6 +78,7 @@ import * as VcsStatusBroadcaster from "./vcs/VcsStatusBroadcaster.ts"; |
| 78 | 78 | import * as GitWorkflowService from "./git/GitWorkflowService.ts"; |
| 79 | 79 | import * as ReviewService from "./review/ReviewService.ts"; |
| 80 | 80 | import * as SourceControlProviderRegistry from "./sourceControl/SourceControlProviderRegistry.ts"; |
| 81 | import * as SourceControlRateLimit from "./sourceControl/SourceControlRateLimit.ts"; | |
| 81 | 82 | import * as SourceControlRepositoryService from "./sourceControl/SourceControlRepositoryService.ts"; |
| 82 | 83 | import * as ProjectSetupScriptRunner from "./project/ProjectSetupScriptRunner.ts"; |
| 83 | 84 | import { ObservabilityLive } from "./observability/Layers/Observability.ts"; |
| ... | ... | @@ -439,6 +440,7 @@ const PullRequestServiceLive = PullRequestService.layer.pipe( |
| 439 | 440 | // One registry entry per supported host; the service only knows the registry. |
| 440 | 441 | Layer.provide(PullRequestProviderRegistry.layer), |
| 441 | 442 | Layer.provide(SourceControlProviderRegistryLayerLive), |
| 443 | Layer.provide(SourceControlRateLimit.layer), | |
| 442 | 444 | Layer.provide(VcsProcess.layer), |
| 443 | 445 | ); |
| 444 | 446 |
apps/server/src/sourceControl/AzureDevOpsCli.test.ts+21| ... | ... | @@ -405,4 +405,25 @@ describe("AzureDevOpsCli.layer", () => { |
| 405 | 405 | ); |
| 406 | 406 | }).pipe(Effect.provide(layer)), |
| 407 | 407 | ); |
| 408 | ||
| 409 | it.effect("preserves rate-limit failures as a distinct error", () => | |
| 410 | Effect.gen(function* () { | |
| 411 | const cause = new VcsProcessExitError({ | |
| 412 | operation: "AzureDevOpsCli.execute", | |
| 413 | command: "az", | |
| 414 | cwd: "/repo", | |
| 415 | argumentCount: 2, | |
| 416 | exitCode: 1, | |
| 417 | detail: "API rate limit exceeded.", | |
| 418 | failureKind: "rate-limited", | |
| 419 | }); | |
| 420 | mockRun.mockReturnValueOnce(Effect.fail(cause)); | |
| 421 | ||
| 422 | const az = yield* AzureDevOpsCli.AzureDevOpsCli; | |
| 423 | const error = yield* az.execute({ cwd: "/repo", args: ["repos", "list"] }).pipe(Effect.flip); | |
| 424 | ||
| 425 | assert.instanceOf(error, AzureDevOpsCli.AzureDevOpsCliRateLimitError); | |
| 426 | assert.strictEqual(error.cause, cause); | |
| 427 | }).pipe(Effect.provide(layer)), | |
| 428 | ); | |
| 408 | 429 | }); |
apps/server/src/sourceControl/AzureDevOpsCli.ts+17| ... | ... | @@ -55,6 +55,19 @@ export class AzureDevOpsCliAuthenticationError extends Schema.TaggedErrorClass<A |
| 55 | 55 | } |
| 56 | 56 | } |
| 57 | 57 | |
| 58 | export class AzureDevOpsCliRateLimitError extends Schema.TaggedErrorClass<AzureDevOpsCliRateLimitError>()( | |
| 59 | "AzureDevOpsCliRateLimitError", | |
| 60 | azureDevOpsCommandErrorFields, | |
| 61 | ) { | |
| 62 | get detail(): string { | |
| 63 | return "Azure DevOps API rate limit exceeded."; | |
| 64 | } | |
| 65 | ||
| 66 | override get message(): string { | |
| 67 | return `Azure DevOps CLI failed in ${this.operation}: ${this.detail}`; | |
| 68 | } | |
| 69 | } | |
| 70 | ||
| 58 | 71 | export class AzureDevOpsPullRequestNotFoundError extends Schema.TaggedErrorClass<AzureDevOpsPullRequestNotFoundError>()( |
| 59 | 72 | "AzureDevOpsPullRequestNotFoundError", |
| 60 | 73 | azureDevOpsCommandErrorFields, |
| ... | ... | @@ -105,6 +118,9 @@ export class AzureDevOpsCommandFailedError extends Schema.TaggedErrorClass<Azure |
| 105 | 118 | if (cause.failureKind === "authentication") { |
| 106 | 119 | return new AzureDevOpsCliAuthenticationError(fields); |
| 107 | 120 | } |
| 121 | if (cause.failureKind === "rate-limited") { | |
| 122 | return new AzureDevOpsCliRateLimitError(fields); | |
| 123 | } | |
| 108 | 124 | if (cause.failureKind === "not-found") { |
| 109 | 125 | return new AzureDevOpsPullRequestNotFoundError(fields); |
| 110 | 126 | } |
| ... | ... | @@ -178,6 +194,7 @@ export class AzureDevOpsRepositoryDecodeError extends Schema.TaggedErrorClass<Az |
| 178 | 194 | export const AzureDevOpsCliError = Schema.Union([ |
| 179 | 195 | AzureDevOpsCliUnavailableError, |
| 180 | 196 | AzureDevOpsCliAuthenticationError, |
| 197 | AzureDevOpsCliRateLimitError, | |
| 181 | 198 | AzureDevOpsPullRequestNotFoundError, |
| 182 | 199 | AzureDevOpsCommandFailedError, |
| 183 | 200 | AzureDevOpsPullRequestListDecodeError, |
apps/server/src/sourceControl/BitbucketApi.test.ts+19| ... | ... | @@ -6,6 +6,7 @@ import * as Effect from "effect/Effect"; |
| 6 | 6 | import * as FileSystem from "effect/FileSystem"; |
| 7 | 7 | import * as Layer from "effect/Layer"; |
| 8 | 8 | import * as Option from "effect/Option"; |
| 9 | import * as TestClock from "effect/testing/TestClock"; | |
| 9 | 10 | import { |
| 10 | 11 | HttpClient, |
| 11 | 12 | HttpClientError, |
| ... | ... | @@ -569,6 +570,24 @@ it.effect("keeps Bitbucket response bodies out of checkout diagnostics", () => { |
| 569 | 570 | }).pipe(Effect.provide(layer)); |
| 570 | 571 | }); |
| 571 | 572 | |
| 573 | it.effect("keeps a 429 Retry-After time on the response error", () => { | |
| 574 | const { layer } = makeLayer({ | |
| 575 | response: () => new Response("busy", { status: 429, headers: { "Retry-After": "120" } }), | |
| 576 | }); | |
| 577 | ||
| 578 | return Effect.gen(function* () { | |
| 579 | yield* TestClock.setTime(1_000); | |
| 580 | const bitbucket = yield* BitbucketApi.BitbucketApi; | |
| 581 | const error = yield* bitbucket | |
| 582 | .request({ method: "GET", url: "/repositories/acme/web" }) | |
| 583 | .pipe(Effect.flip); | |
| 584 | ||
| 585 | assert.instanceOf(error, BitbucketApi.BitbucketResponseError); | |
| 586 | assert.strictEqual(error.status, 429); | |
| 587 | assert.strictEqual(error.retryAt, 121_000); | |
| 588 | }).pipe(Effect.provide(layer)); | |
| 589 | }); | |
| 590 | ||
| 572 | 591 | it.effect("preserves Bitbucket response body read failures as their immediate cause", () => { |
| 573 | 592 | const cause = new Error("response stream failed"); |
| 574 | 593 | const { layer } = makeLayer({ |
apps/server/src/sourceControl/BitbucketApi.ts+24-21| ... | ... | @@ -1,3 +1,4 @@ |
| 1 | import * as Clock from "effect/Clock"; | |
| 1 | 2 | import * as Config from "effect/Config"; |
| 2 | 3 | import * as Context from "effect/Context"; |
| 3 | 4 | import * as Effect from "effect/Effect"; |
| ... | ... | @@ -29,6 +30,7 @@ import { collectUint8StreamText } from "../stream/collectUint8StreamText.ts"; |
| 29 | 30 | import * as SourceControlProvider from "./SourceControlProvider.ts"; |
| 30 | 31 | import * as GitVcsDriver from "../vcs/GitVcsDriver.ts"; |
| 31 | 32 | import * as VcsDriverRegistry from "../vcs/VcsDriverRegistry.ts"; |
| 33 | import { retryAtFromHeader } from "./SourceControlRateLimit.ts"; | |
| 32 | 34 | |
| 33 | 35 | const DEFAULT_API_BASE_URL = "https://api.bitbucket.org/2.0"; |
| 34 | 36 | /** A response body past this is cut short, so one huge diff cannot exhaust the server. */ |
| ... | ... | @@ -98,6 +100,7 @@ export class BitbucketResponseError extends Schema.TaggedErrorClass<BitbucketRes |
| 98 | 100 | operation: BitbucketApiOperation, |
| 99 | 101 | status: Schema.Int, |
| 100 | 102 | responseBodyLength: NonNegativeInt, |
| 103 | retryAt: Schema.optional(Schema.Number), | |
| 101 | 104 | }, |
| 102 | 105 | ) { |
| 103 | 106 | get detail(): string { |
| ... | ... | @@ -579,28 +582,28 @@ function responseError( |
| 579 | 582 | ): Effect.Effect<never, BitbucketApiError> { |
| 580 | 583 | // Bounded like any other body: an error response is no smaller than a successful one, and |
| 581 | 584 | // only its length is reported anyway. |
| 582 | return collectUint8StreamText({ | |
| 583 | stream: response.stream, | |
| 584 | maxBytes: DEFAULT_MAX_RESPONSE_BYTES, | |
| 585 | }).pipe( | |
| 586 | Effect.mapError( | |
| 587 | (cause) => | |
| 588 | new BitbucketResponseBodyReadError({ | |
| 589 | operation, | |
| 590 | status: response.status, | |
| 591 | cause, | |
| 592 | }), | |
| 593 | ), | |
| 594 | Effect.flatMap((collected) => | |
| 595 | Effect.fail( | |
| 596 | new BitbucketResponseError({ | |
| 597 | operation, | |
| 598 | status: response.status, | |
| 599 | responseBodyLength: collected.text.length, | |
| 600 | }), | |
| 585 | return Effect.gen(function* () { | |
| 586 | const now = yield* Clock.currentTimeMillis; | |
| 587 | const collected = yield* collectUint8StreamText({ | |
| 588 | stream: response.stream, | |
| 589 | maxBytes: DEFAULT_MAX_RESPONSE_BYTES, | |
| 590 | }).pipe( | |
| 591 | Effect.mapError( | |
| 592 | (cause) => | |
| 593 | new BitbucketResponseBodyReadError({ | |
| 594 | operation, | |
| 595 | status: response.status, | |
| 596 | cause, | |
| 597 | }), | |
| 601 | 598 | ), |
| 602 | ), | |
| 603 | ); | |
| 599 | ); | |
| 600 | return yield* new BitbucketResponseError({ | |
| 601 | operation, | |
| 602 | status: response.status, | |
| 603 | responseBodyLength: collected.text.length, | |
| 604 | retryAt: retryAtFromHeader(response.headers["retry-after"], now), | |
| 605 | }); | |
| 606 | }); | |
| 604 | 607 | } |
| 605 | 608 | |
| 606 | 609 | export const make = Effect.gen(function* () { |
apps/server/src/sourceControl/GitLabCli.test.ts+22| ... | ... | @@ -364,4 +364,26 @@ layer("GitLabCli.layer", (it) => { |
| 364 | 364 | assert.strictEqual(error.cause, cause); |
| 365 | 365 | }), |
| 366 | 366 | ); |
| 367 | ||
| 368 | it.effect("preserves rate-limit failures as a distinct error", () => | |
| 369 | Effect.gen(function* () { | |
| 370 | const cause = new VcsProcessExitError({ | |
| 371 | operation: "GitLabCli.execute", | |
| 372 | command: "glab", | |
| 373 | cwd: "/repo", | |
| 374 | exitCode: 1, | |
| 375 | detail: "API rate limit exceeded.", | |
| 376 | failureKind: "rate-limited", | |
| 377 | }); | |
| 378 | mockedRun.mockReturnValueOnce(Effect.fail(cause)); | |
| 379 | ||
| 380 | const glab = yield* GitLabCli.GitLabCli; | |
| 381 | const error = yield* glab | |
| 382 | .execute({ cwd: "/repo", args: ["api", "projects"] }) | |
| 383 | .pipe(Effect.flip); | |
| 384 | ||
| 385 | assert.strictEqual(error._tag, "GitLabCliRateLimitError"); | |
| 386 | assert.strictEqual(error.cause, cause); | |
| 387 | }), | |
| 388 | ); | |
| 367 | 389 | }); |
apps/server/src/sourceControl/GitLabCli.ts+16-1| ... | ... | @@ -61,6 +61,19 @@ export class GitLabCliAuthenticationError extends Schema.TaggedErrorClass<GitLab |
| 61 | 61 | } |
| 62 | 62 | } |
| 63 | 63 | |
| 64 | export class GitLabCliRateLimitError extends Schema.TaggedErrorClass<GitLabCliRateLimitError>()( | |
| 65 | "GitLabCliRateLimitError", | |
| 66 | gitLabCliExecutionErrorContext, | |
| 67 | ) { | |
| 68 | get detail(): string { | |
| 69 | return "GitLab API rate limit exceeded."; | |
| 70 | } | |
| 71 | ||
| 72 | override get message(): string { | |
| 73 | return `GitLab CLI failed in ${this.operation}: ${this.detail}`; | |
| 74 | } | |
| 75 | } | |
| 76 | ||
| 64 | 77 | export class GitLabMergeRequestNotFoundError extends Schema.TaggedErrorClass<GitLabMergeRequestNotFoundError>()( |
| 65 | 78 | "GitLabMergeRequestNotFoundError", |
| 66 | 79 | { |
| ... | ... | @@ -126,8 +139,9 @@ export class GitLabCliCommandError extends Schema.TaggedErrorClass<GitLabCliComm |
| 126 | 139 | switch (cause.failureKind) { |
| 127 | 140 | case "authentication": |
| 128 | 141 | return new GitLabCliAuthenticationError({ ...context, cause }); |
| 129 | case "not-found": | |
| 130 | 142 | case "rate-limited": |
| 143 | return new GitLabCliRateLimitError({ ...context, cause }); | |
| 144 | case "not-found": | |
| 131 | 145 | case "command-failed": |
| 132 | 146 | case undefined: |
| 133 | 147 | return new GitLabCliCommandError({ ...context, cause }); |
| ... | ... | @@ -214,6 +228,7 @@ export class GitLabNamespaceDecodeError extends Schema.TaggedErrorClass<GitLabNa |
| 214 | 228 | export const GitLabCliError = Schema.Union([ |
| 215 | 229 | GitLabCliUnavailableError, |
| 216 | 230 | GitLabCliAuthenticationError, |
| 231 | GitLabCliRateLimitError, | |
| 217 | 232 | GitLabMergeRequestNotFoundError, |
| 218 | 233 | GitLabCliCommandError, |
| 219 | 234 | GitLabMergeRequestListDecodeError, |
apps/server/src/sourceControl/SourceControlRateLimit.test.ts created+122| ... | ... | @@ -0,0 +1,122 @@ |
| 1 | import { assert, it } from "@effect/vitest"; | |
| 2 | import * as Effect from "effect/Effect"; | |
| 3 | import * as TestClock from "effect/testing/TestClock"; | |
| 4 | ||
| 5 | import * as SourceControlRateLimit from "./SourceControlRateLimit.ts"; | |
| 6 | ||
| 7 | const github = { provider: "github" as const, host: "github.com" }; | |
| 8 | ||
| 9 | it("parses Retry-After seconds and HTTP dates", () => { | |
| 10 | assert.equal(SourceControlRateLimit.retryAtFromHeader("120", 1_000), 121_000); | |
| 11 | assert.equal( | |
| 12 | SourceControlRateLimit.retryAtFromHeader("Thu, 01 Jan 1970 00:02:01 GMT", 1_000), | |
| 13 | 121_000, | |
| 14 | ); | |
| 15 | assert.isUndefined(SourceControlRateLimit.retryAtFromHeader("later", 1_000)); | |
| 16 | }); | |
| 17 | ||
| 18 | it.effect("backs off repeated rate limits until a successful request", () => | |
| 19 | Effect.gen(function* () { | |
| 20 | yield* TestClock.setTime(0); | |
| 21 | const limits = yield* SourceControlRateLimit.SourceControlRateLimit; | |
| 22 | ||
| 23 | const firstLease = yield* limits.check(github); | |
| 24 | yield* limits.recordRateLimit({ ...github, lease: firstLease }); | |
| 25 | const firstPause = yield* Effect.flip(limits.check(github)); | |
| 26 | assert.deepInclude(firstPause, { | |
| 27 | _tag: "SourceControlRateLimitPausedError", | |
| 28 | provider: "github", | |
| 29 | host: "github.com", | |
| 30 | retryAt: 30_000, | |
| 31 | }); | |
| 32 | assert.equal( | |
| 33 | firstPause.detail, | |
| 34 | "github requests to github.com are paused until the rate limit resets.", | |
| 35 | ); | |
| 36 | assert.equal(firstPause.message, firstPause.detail); | |
| 37 | ||
| 38 | yield* TestClock.adjust("30 seconds"); | |
| 39 | const secondLease = yield* limits.check(github); | |
| 40 | yield* limits.recordRateLimit({ ...github, lease: secondLease }); | |
| 41 | assert.equal((yield* Effect.flip(limits.check(github))).retryAt, 90_000); | |
| 42 | ||
| 43 | yield* TestClock.adjust("60 seconds"); | |
| 44 | const successLease = yield* limits.check(github); | |
| 45 | yield* limits.recordSuccess({ ...github, lease: successLease }); | |
| 46 | const resetLease = yield* limits.check(github); | |
| 47 | yield* limits.recordRateLimit({ ...github, lease: resetLease }); | |
| 48 | assert.equal((yield* Effect.flip(limits.check(github))).retryAt, 120_000); | |
| 49 | }).pipe(Effect.provide(SourceControlRateLimit.layer)), | |
| 50 | ); | |
| 51 | ||
| 52 | it.effect("honors a provider reset time", () => | |
| 53 | Effect.gen(function* () { | |
| 54 | yield* TestClock.setTime(1_000); | |
| 55 | const limits = yield* SourceControlRateLimit.SourceControlRateLimit; | |
| 56 | const lease = yield* limits.check(github); | |
| 57 | ||
| 58 | yield* limits.recordRateLimit({ ...github, lease, retryAt: 121_000 }); | |
| 59 | assert.equal((yield* Effect.flip(limits.check(github))).retryAt, 121_000); | |
| 60 | ||
| 61 | yield* TestClock.adjust("119999 millis"); | |
| 62 | assert.equal((yield* Effect.flip(limits.check(github))).retryAt, 121_000); | |
| 63 | yield* TestClock.adjust("1 millis"); | |
| 64 | assert.equal(yield* limits.check(github), 1); | |
| 65 | }).pipe(Effect.provide(SourceControlRateLimit.layer)), | |
| 66 | ); | |
| 67 | ||
| 68 | it.effect("lets an interactive request through without clearing an active pause", () => | |
| 69 | Effect.gen(function* () { | |
| 70 | yield* TestClock.setTime(1_000); | |
| 71 | const limits = yield* SourceControlRateLimit.SourceControlRateLimit; | |
| 72 | const lease = yield* limits.check(github); | |
| 73 | yield* limits.recordRateLimit({ ...github, lease, retryAt: 121_000 }); | |
| 74 | ||
| 75 | const interactiveLease = yield* limits.check(github, { allowPaused: true }); | |
| 76 | yield* limits.recordSuccess({ ...github, lease: interactiveLease }); | |
| 77 | ||
| 78 | assert.equal(interactiveLease, 1); | |
| 79 | assert.equal((yield* Effect.flip(limits.check(github))).retryAt, 121_000); | |
| 80 | ||
| 81 | yield* limits.recordRateLimit({ ...github, lease: interactiveLease, retryAt: 61_000 }); | |
| 82 | assert.equal((yield* Effect.flip(limits.check(github))).retryAt, 121_000); | |
| 83 | }).pipe(Effect.provide(SourceControlRateLimit.layer)), | |
| 84 | ); | |
| 85 | ||
| 86 | it.effect("keeps providers and hosts isolated", () => | |
| 87 | Effect.gen(function* () { | |
| 88 | const limits = yield* SourceControlRateLimit.SourceControlRateLimit; | |
| 89 | const lease = yield* limits.check(github); | |
| 90 | yield* limits.recordRateLimit({ ...github, lease }); | |
| 91 | ||
| 92 | assert.equal(yield* limits.check({ provider: "gitlab", host: "github.com" }), 0); | |
| 93 | assert.equal(yield* limits.check({ provider: "github", host: "github.example.com" }), 0); | |
| 94 | }).pipe(Effect.provide(SourceControlRateLimit.layer)), | |
| 95 | ); | |
| 96 | ||
| 97 | it.effect("does not let an older success clear a concurrent pause", () => | |
| 98 | Effect.gen(function* () { | |
| 99 | const limits = yield* SourceControlRateLimit.SourceControlRateLimit; | |
| 100 | const firstLease = yield* limits.check(github); | |
| 101 | const concurrentLease = yield* limits.check(github); | |
| 102 | ||
| 103 | yield* limits.recordRateLimit({ ...github, lease: firstLease }); | |
| 104 | yield* limits.recordSuccess({ ...github, lease: concurrentLease }); | |
| 105 | ||
| 106 | assert.equal((yield* Effect.flip(limits.check(github))).retryAt, 30_000); | |
| 107 | }).pipe(Effect.provide(SourceControlRateLimit.layer)), | |
| 108 | ); | |
| 109 | ||
| 110 | it.effect("keeps a fresh provider reset from an older request", () => | |
| 111 | Effect.gen(function* () { | |
| 112 | yield* TestClock.setTime(0); | |
| 113 | const limits = yield* SourceControlRateLimit.SourceControlRateLimit; | |
| 114 | const staleLease = yield* limits.check(github); | |
| 115 | yield* limits.recordRateLimit({ ...github, lease: staleLease }); | |
| 116 | ||
| 117 | yield* TestClock.adjust("31 seconds"); | |
| 118 | yield* limits.recordRateLimit({ ...github, lease: staleLease, retryAt: 61_000 }); | |
| 119 | ||
| 120 | assert.equal((yield* Effect.flip(limits.check(github))).retryAt, 61_000); | |
| 121 | }).pipe(Effect.provide(SourceControlRateLimit.layer)), | |
| 122 | ); |
apps/server/src/sourceControl/SourceControlRateLimit.ts created+162| ... | ... | @@ -0,0 +1,162 @@ |
| 1 | import * as Clock from "effect/Clock"; | |
| 2 | import * as Context from "effect/Context"; | |
| 3 | import * as Duration from "effect/Duration"; | |
| 4 | import * as Effect from "effect/Effect"; | |
| 5 | import * as Layer from "effect/Layer"; | |
| 6 | import * as Ref from "effect/Ref"; | |
| 7 | import * as Schema from "effect/Schema"; | |
| 8 | import { | |
| 9 | SourceControlProviderKind as SourceControlProviderKindSchema, | |
| 10 | type SourceControlProviderKind, | |
| 11 | } from "@t3tools/contracts"; | |
| 12 | ||
| 13 | const FALLBACK_COOLDOWN = Duration.seconds(30); | |
| 14 | const MAX_FALLBACK_COOLDOWN = Duration.minutes(15); | |
| 15 | ||
| 16 | interface RateLimitKey { | |
| 17 | readonly provider: SourceControlProviderKind; | |
| 18 | readonly host: string; | |
| 19 | } | |
| 20 | ||
| 21 | interface RateLimitLease extends RateLimitKey { | |
| 22 | readonly lease: number; | |
| 23 | } | |
| 24 | ||
| 25 | interface RateLimitEntry { | |
| 26 | readonly attempt: number; | |
| 27 | readonly generation: number; | |
| 28 | readonly retryAt: number; | |
| 29 | } | |
| 30 | ||
| 31 | export class SourceControlRateLimitPausedError extends Schema.TaggedErrorClass<SourceControlRateLimitPausedError>()( | |
| 32 | "SourceControlRateLimitPausedError", | |
| 33 | { | |
| 34 | provider: SourceControlProviderKindSchema, | |
| 35 | host: Schema.String, | |
| 36 | retryAt: Schema.Number, | |
| 37 | }, | |
| 38 | ) { | |
| 39 | get detail(): string { | |
| 40 | return `${this.provider} requests to ${this.host} are paused until the rate limit resets.`; | |
| 41 | } | |
| 42 | ||
| 43 | override get message(): string { | |
| 44 | return this.detail; | |
| 45 | } | |
| 46 | } | |
| 47 | ||
| 48 | export class SourceControlRateLimit extends Context.Service< | |
| 49 | SourceControlRateLimit, | |
| 50 | { | |
| 51 | readonly check: ( | |
| 52 | key: RateLimitKey, | |
| 53 | options?: { readonly allowPaused: boolean }, | |
| 54 | ) => Effect.Effect<number, SourceControlRateLimitPausedError>; | |
| 55 | readonly recordRateLimit: ( | |
| 56 | input: RateLimitLease & { readonly retryAt?: number | undefined }, | |
| 57 | ) => Effect.Effect<void>; | |
| 58 | readonly recordSuccess: (input: RateLimitLease) => Effect.Effect<void>; | |
| 59 | } | |
| 60 | >()("t3/sourceControl/SourceControlRateLimit") {} | |
| 61 | ||
| 62 | function normalizedKey(key: RateLimitKey): string { | |
| 63 | return `${key.provider}\0${key.host.trim().toLowerCase()}`; | |
| 64 | } | |
| 65 | ||
| 66 | function fallbackCooldownMs(attempt: number): number { | |
| 67 | return Math.min( | |
| 68 | Duration.toMillis(FALLBACK_COOLDOWN) * 2 ** Math.max(0, attempt - 1), | |
| 69 | Duration.toMillis(MAX_FALLBACK_COOLDOWN), | |
| 70 | ); | |
| 71 | } | |
| 72 | ||
| 73 | export function retryAtFromHeader(value: string | undefined, now: number): number | undefined { | |
| 74 | if (value === undefined) return undefined; | |
| 75 | const normalized = value.trim(); | |
| 76 | if (/^\d+$/u.test(normalized)) { | |
| 77 | const seconds = Number(normalized); | |
| 78 | const retryAt = now + seconds * 1_000; | |
| 79 | return Number.isSafeInteger(retryAt) ? retryAt : undefined; | |
| 80 | } | |
| 81 | const retryAt = Date.parse(normalized); | |
| 82 | return Number.isFinite(retryAt) && retryAt > now ? retryAt : undefined; | |
| 83 | } | |
| 84 | ||
| 85 | export const make = Effect.gen(function* () { | |
| 86 | const entries = yield* Ref.make<ReadonlyMap<string, RateLimitEntry>>(new Map()); | |
| 87 | ||
| 88 | const check: SourceControlRateLimit["Service"]["check"] = Effect.fn( | |
| 89 | "SourceControlRateLimit.check", | |
| 90 | )(function* (input, options) { | |
| 91 | const now = yield* Clock.currentTimeMillis; | |
| 92 | const entry = (yield* Ref.get(entries)).get(normalizedKey(input)); | |
| 93 | if (entry !== undefined && entry.retryAt > now && options?.allowPaused !== true) { | |
| 94 | return yield* new SourceControlRateLimitPausedError({ | |
| 95 | provider: input.provider, | |
| 96 | host: input.host.trim().toLowerCase(), | |
| 97 | retryAt: entry.retryAt, | |
| 98 | }); | |
| 99 | } | |
| 100 | return entry?.generation ?? 0; | |
| 101 | }); | |
| 102 | ||
| 103 | const recordRateLimit: SourceControlRateLimit["Service"]["recordRateLimit"] = Effect.fn( | |
| 104 | "SourceControlRateLimit.recordRateLimit", | |
| 105 | )(function* (input) { | |
| 106 | const now = yield* Clock.currentTimeMillis; | |
| 107 | yield* Ref.update(entries, (current) => { | |
| 108 | const key = normalizedKey(input); | |
| 109 | const previous = current.get(key); | |
| 110 | if (previous !== undefined && previous.generation > input.lease) { | |
| 111 | if (previous.retryAt <= now && (input.retryAt === undefined || input.retryAt <= now)) { | |
| 112 | return current; | |
| 113 | } | |
| 114 | const retryAt = | |
| 115 | input.retryAt !== undefined && input.retryAt > previous.retryAt | |
| 116 | ? input.retryAt | |
| 117 | : previous.retryAt; | |
| 118 | if (retryAt === previous.retryAt) return current; | |
| 119 | const next = new Map(current); | |
| 120 | next.set(key, { ...previous, retryAt }); | |
| 121 | return next; | |
| 122 | } | |
| 123 | ||
| 124 | const attempt = (previous?.attempt ?? 0) + 1; | |
| 125 | const proposedRetryAt = | |
| 126 | input.retryAt !== undefined && input.retryAt > now | |
| 127 | ? input.retryAt | |
| 128 | : now + fallbackCooldownMs(attempt); | |
| 129 | const retryAt = | |
| 130 | previous !== undefined && previous.retryAt > now | |
| 131 | ? Math.max(previous.retryAt, proposedRetryAt) | |
| 132 | : proposedRetryAt; | |
| 133 | const next = new Map(current); | |
| 134 | next.set(key, { | |
| 135 | attempt, | |
| 136 | generation: Math.max(previous?.generation ?? 0, input.lease) + 1, | |
| 137 | retryAt, | |
| 138 | }); | |
| 139 | return next; | |
| 140 | }); | |
| 141 | }); | |
| 142 | ||
| 143 | const recordSuccess: SourceControlRateLimit["Service"]["recordSuccess"] = Effect.fn( | |
| 144 | "SourceControlRateLimit.recordSuccess", | |
| 145 | )(function* (input) { | |
| 146 | const now = yield* Clock.currentTimeMillis; | |
| 147 | yield* Ref.update(entries, (current) => { | |
| 148 | const key = normalizedKey(input); | |
| 149 | const previous = current.get(key); | |
| 150 | if (previous === undefined || previous.generation !== input.lease || previous.retryAt > now) { | |
| 151 | return current; | |
| 152 | } | |
| 153 | const next = new Map(current); | |
| 154 | next.set(key, { attempt: 0, generation: previous.generation, retryAt: 0 }); | |
| 155 | return next; | |
| 156 | }); | |
| 157 | }); | |
| 158 | ||
| 159 | return SourceControlRateLimit.of({ check, recordRateLimit, recordSuccess }); | |
| 160 | }); | |
| 161 | ||
| 162 | export const layer = Layer.effect(SourceControlRateLimit, make); |
apps/server/src/sourceControl/githubGraphQlBudget.test.ts created+189| ... | ... | @@ -0,0 +1,189 @@ |
| 1 | import { describe, expect, it } from "@effect/vitest"; | |
| 2 | import * as Effect from "effect/Effect"; | |
| 3 | import * as TestClock from "effect/testing/TestClock"; | |
| 4 | ||
| 5 | import * as GitHubGraphQlBudget from "./githubGraphQlBudget.ts"; | |
| 6 | ||
| 7 | const RESET_AT = "2026-08-13T14:00:00.000Z"; | |
| 8 | const NEXT_RESET_AT = "2026-08-13T15:00:00.000Z"; | |
| 9 | const BEFORE_RESET = Date.parse("2026-08-13T13:30:00.000Z"); | |
| 10 | const AFTER_RESET = Date.parse("2026-08-13T14:00:01.000Z"); | |
| 11 | ||
| 12 | function rateLimit(remaining: number, limit = 5_000, resetAt = RESET_AT): string { | |
| 13 | return JSON.stringify({ | |
| 14 | data: { | |
| 15 | viewer: { login: "bilal" }, | |
| 16 | rateLimit: { cost: 14, limit, remaining, resetAt }, | |
| 17 | }, | |
| 18 | }); | |
| 19 | } | |
| 20 | ||
| 21 | describe("GitHub GraphQL budget", () => { | |
| 22 | it.effect("adds rate metadata to a read query", () => | |
| 23 | Effect.gen(function* () { | |
| 24 | const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; | |
| 25 | ||
| 26 | const query = yield* budget.query( | |
| 27 | "github.com", | |
| 28 | 'query { repository(owner: "acme", name: "web") { name } }', | |
| 29 | ); | |
| 30 | ||
| 31 | expect(query).toContain("rateLimit { cost limit remaining resetAt }"); | |
| 32 | expect(query).toContain('repository(owner: "acme", name: "web") { name }'); | |
| 33 | }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), | |
| 34 | ); | |
| 35 | ||
| 36 | it.effect("protects the last ten percent with the shared provider cooldown error", () => | |
| 37 | Effect.gen(function* () { | |
| 38 | yield* TestClock.setTime(BEFORE_RESET); | |
| 39 | const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; | |
| 40 | yield* budget.observe("github.com", rateLimit(500)); | |
| 41 | ||
| 42 | const error = yield* Effect.flip(budget.query("github.com", "query { viewer { login } }")); | |
| 43 | expect(error).toMatchObject({ | |
| 44 | _tag: "SourceControlRateLimitPausedError", | |
| 45 | provider: "github", | |
| 46 | host: "github.com", | |
| 47 | retryAt: Date.parse(RESET_AT), | |
| 48 | }); | |
| 49 | expect(error.message).toBe( | |
| 50 | "github requests to github.com are paused until the rate limit resets.", | |
| 51 | ); | |
| 52 | ||
| 53 | yield* TestClock.setTime(AFTER_RESET); | |
| 54 | expect(yield* budget.query("github.com", "query { viewer { login } }")).toContain( | |
| 55 | "rateLimit", | |
| 56 | ); | |
| 57 | }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), | |
| 58 | ); | |
| 59 | ||
| 60 | it.effect("keeps hosts isolated", () => | |
| 61 | Effect.gen(function* () { | |
| 62 | yield* TestClock.setTime(BEFORE_RESET); | |
| 63 | const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; | |
| 64 | yield* budget.observe("github.com", rateLimit(0)); | |
| 65 | ||
| 66 | const error = yield* Effect.flip(budget.query("github.com", "query { viewer { login } }")); | |
| 67 | expect(error._tag).toBe("SourceControlRateLimitPausedError"); | |
| 68 | expect(yield* budget.query("github.example.com", "query { viewer { login } }")).toContain( | |
| 69 | "rateLimit", | |
| 70 | ); | |
| 71 | }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), | |
| 72 | ); | |
| 73 | ||
| 74 | it.effect("keeps the lower remaining value from out-of-order responses", () => | |
| 75 | Effect.gen(function* () { | |
| 76 | yield* TestClock.setTime(BEFORE_RESET); | |
| 77 | const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; | |
| 78 | yield* budget.observe("github.com", rateLimit(400)); | |
| 79 | yield* budget.observe("github.com", rateLimit(600)); | |
| 80 | ||
| 81 | const error = yield* Effect.flip(budget.query("github.com", "query { viewer { login } }")); | |
| 82 | expect(error).toMatchObject({ | |
| 83 | _tag: "SourceControlRateLimitPausedError", | |
| 84 | retryAt: Date.parse(RESET_AT), | |
| 85 | }); | |
| 86 | }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), | |
| 87 | ); | |
| 88 | ||
| 89 | it.effect("ignores a response from an older reset window", () => | |
| 90 | Effect.gen(function* () { | |
| 91 | yield* TestClock.setTime(BEFORE_RESET); | |
| 92 | const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; | |
| 93 | yield* budget.observe("github.com", rateLimit(400, 5_000, NEXT_RESET_AT)); | |
| 94 | yield* budget.observe("github.com", rateLimit(1_000, 5_000, RESET_AT)); | |
| 95 | ||
| 96 | const error = yield* Effect.flip(budget.query("github.com", "query { viewer { login } }")); | |
| 97 | expect(error).toMatchObject({ | |
| 98 | _tag: "SourceControlRateLimitPausedError", | |
| 99 | retryAt: Date.parse(NEXT_RESET_AT), | |
| 100 | }); | |
| 101 | }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), | |
| 102 | ); | |
| 103 | ||
| 104 | it.effect("accepts a response from a later reset window", () => | |
| 105 | Effect.gen(function* () { | |
| 106 | yield* TestClock.setTime(BEFORE_RESET); | |
| 107 | const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; | |
| 108 | yield* budget.observe("github.com", rateLimit(1_000)); | |
| 109 | yield* budget.observe("github.com", rateLimit(400, 5_000, NEXT_RESET_AT)); | |
| 110 | ||
| 111 | const error = yield* Effect.flip(budget.query("github.com", "query { viewer { login } }")); | |
| 112 | expect(error).toMatchObject({ | |
| 113 | _tag: "SourceControlRateLimitPausedError", | |
| 114 | retryAt: Date.parse(NEXT_RESET_AT), | |
| 115 | }); | |
| 116 | }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), | |
| 117 | ); | |
| 118 | ||
| 119 | it.effect("allows reads above the reserve", () => | |
| 120 | Effect.gen(function* () { | |
| 121 | yield* TestClock.setTime(BEFORE_RESET); | |
| 122 | const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; | |
| 123 | yield* budget.observe("github.com", rateLimit(515)); | |
| 124 | ||
| 125 | expect(yield* budget.query("github.com", "query { viewer { login } }")).toContain( | |
| 126 | "rateLimit", | |
| 127 | ); | |
| 128 | }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), | |
| 129 | ); | |
| 130 | ||
| 131 | it.effect("allows an interactive read to use the reserve", () => | |
| 132 | Effect.gen(function* () { | |
| 133 | yield* TestClock.setTime(BEFORE_RESET); | |
| 134 | const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; | |
| 135 | yield* budget.observe("github.com", rateLimit(500)); | |
| 136 | ||
| 137 | expect( | |
| 138 | yield* budget.query("github.com", "query { viewer { login } }", { | |
| 139 | allowReserve: true, | |
| 140 | }), | |
| 141 | ).toContain("rateLimit"); | |
| 142 | }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), | |
| 143 | ); | |
| 144 | ||
| 145 | it.effect("reserves an admitted query cost before its response", () => | |
| 146 | Effect.gen(function* () { | |
| 147 | yield* TestClock.setTime(BEFORE_RESET); | |
| 148 | const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; | |
| 149 | yield* budget.observe("github.com", rateLimit(514)); | |
| 150 | ||
| 151 | expect(yield* budget.query("github.com", "query { viewer { login } }")).toContain( | |
| 152 | "rateLimit", | |
| 153 | ); | |
| 154 | const error = yield* Effect.flip(budget.query("github.com", "query { viewer { login } }")); | |
| 155 | ||
| 156 | expect(error).toMatchObject({ | |
| 157 | _tag: "SourceControlRateLimitPausedError", | |
| 158 | retryAt: Date.parse(RESET_AT), | |
| 159 | }); | |
| 160 | }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), | |
| 161 | ); | |
| 162 | ||
| 163 | it.effect("ignores malformed or partial rate metadata", () => | |
| 164 | Effect.gen(function* () { | |
| 165 | yield* TestClock.setTime(BEFORE_RESET); | |
| 166 | const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; | |
| 167 | yield* budget.observe("github.com", "{"); | |
| 168 | yield* budget.observe( | |
| 169 | "github.com", | |
| 170 | '{"data":{"rateLimit":{"limit":0,"remaining":-1,"resetAt":"never"}}}', | |
| 171 | ); | |
| 172 | ||
| 173 | expect(yield* budget.query("github.com", "query { viewer { login } }")).toContain( | |
| 174 | "rateLimit", | |
| 175 | ); | |
| 176 | }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), | |
| 177 | ); | |
| 178 | ||
| 179 | it.effect("does not add a read field to a mutation", () => | |
| 180 | Effect.gen(function* () { | |
| 181 | yield* TestClock.setTime(BEFORE_RESET); | |
| 182 | const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget; | |
| 183 | const mutation = "mutation { addComment(input: {}) { clientMutationId } }"; | |
| 184 | yield* budget.observe("github.com", rateLimit(0)); | |
| 185 | ||
| 186 | expect(yield* budget.query("github.com", mutation)).toBe(mutation); | |
| 187 | }).pipe(Effect.provide(GitHubGraphQlBudget.layer)), | |
| 188 | ); | |
| 189 | }); |
apps/server/src/sourceControl/githubGraphQlBudget.ts created+141| ... | ... | @@ -0,0 +1,141 @@ |
| 1 | import * as Clock from "effect/Clock"; | |
| 2 | import * as Context from "effect/Context"; | |
| 3 | import * as Effect from "effect/Effect"; | |
| 4 | import * as Layer from "effect/Layer"; | |
| 5 | import * as Ref from "effect/Ref"; | |
| 6 | import * as Predicate from "effect/Predicate"; | |
| 7 | ||
| 8 | import * as SourceControlRateLimit from "./SourceControlRateLimit.ts"; | |
| 9 | ||
| 10 | const GRAPHQL_RESERVE_RATIO = 0.1; | |
| 11 | const RATE_LIMIT_SELECTION = "rateLimit { cost limit remaining resetAt }"; | |
| 12 | ||
| 13 | interface GraphQlBudgetSnapshot { | |
| 14 | readonly cost: number; | |
| 15 | readonly limit: number; | |
| 16 | readonly remaining: number; | |
| 17 | readonly resetAtMs: number; | |
| 18 | } | |
| 19 | ||
| 20 | export class GitHubGraphQlBudget extends Context.Service< | |
| 21 | GitHubGraphQlBudget, | |
| 22 | { | |
| 23 | readonly query: ( | |
| 24 | host: string, | |
| 25 | document: string, | |
| 26 | options?: { readonly allowReserve: boolean }, | |
| 27 | ) => Effect.Effect<string, SourceControlRateLimit.SourceControlRateLimitPausedError>; | |
| 28 | readonly observe: (host: string, raw: string) => Effect.Effect<void>; | |
| 29 | } | |
| 30 | >()("t3/sourceControl/githubGraphQlBudget") {} | |
| 31 | ||
| 32 | function hostKey(host: string): string { | |
| 33 | return host.trim().toLowerCase(); | |
| 34 | } | |
| 35 | ||
| 36 | function snapshotFrom(raw: string): GraphQlBudgetSnapshot | null { | |
| 37 | try { | |
| 38 | const parsed: unknown = JSON.parse(raw); | |
| 39 | if ( | |
| 40 | !Predicate.isObject(parsed) || | |
| 41 | !Predicate.isObject(parsed.data) || | |
| 42 | !Predicate.isObject(parsed.data.rateLimit) | |
| 43 | ) { | |
| 44 | return null; | |
| 45 | } | |
| 46 | const { cost, limit, remaining, resetAt } = parsed.data.rateLimit; | |
| 47 | if ( | |
| 48 | typeof cost !== "number" || | |
| 49 | !Number.isFinite(cost) || | |
| 50 | cost < 0 || | |
| 51 | typeof limit !== "number" || | |
| 52 | !Number.isFinite(limit) || | |
| 53 | limit <= 0 || | |
| 54 | typeof remaining !== "number" || | |
| 55 | !Number.isFinite(remaining) || | |
| 56 | remaining < 0 || | |
| 57 | typeof resetAt !== "string" | |
| 58 | ) { | |
| 59 | return null; | |
| 60 | } | |
| 61 | const resetAtMs = Date.parse(resetAt); | |
| 62 | return Number.isFinite(resetAtMs) ? { cost, limit, remaining, resetAtMs } : null; | |
| 63 | } catch { | |
| 64 | return null; | |
| 65 | } | |
| 66 | } | |
| 67 | ||
| 68 | function isReadOperation(document: string): boolean { | |
| 69 | const operation = document.trimStart(); | |
| 70 | return operation.startsWith("query") || operation.startsWith("{"); | |
| 71 | } | |
| 72 | ||
| 73 | function withRateLimit(document: string): string { | |
| 74 | if (!isReadOperation(document)) return document; | |
| 75 | const end = document.lastIndexOf("}"); | |
| 76 | if (end === -1 || document.includes(RATE_LIMIT_SELECTION)) return document; | |
| 77 | return `${document.slice(0, end)}\n ${RATE_LIMIT_SELECTION}\n${document.slice(end)}`; | |
| 78 | } | |
| 79 | ||
| 80 | export const make = Effect.gen(function* () { | |
| 81 | const snapshots = yield* Ref.make<ReadonlyMap<string, GraphQlBudgetSnapshot>>(new Map()); | |
| 82 | ||
| 83 | const query: GitHubGraphQlBudget["Service"]["query"] = Effect.fn("GitHubGraphQlBudget.query")( | |
| 84 | function* (host, document, options) { | |
| 85 | if (!isReadOperation(document)) return document; | |
| 86 | const now = yield* Clock.currentTimeMillis; | |
| 87 | const retryAt = yield* Ref.modify(snapshots, (current) => { | |
| 88 | const key = hostKey(host); | |
| 89 | const snapshot = current.get(key); | |
| 90 | if (snapshot === undefined) return [null, current] as const; | |
| 91 | if (snapshot.resetAtMs <= now) { | |
| 92 | const next = new Map(current); | |
| 93 | next.delete(key); | |
| 94 | return [null, next] as const; | |
| 95 | } | |
| 96 | const remaining = Math.max(0, snapshot.remaining - Math.max(1, snapshot.cost)); | |
| 97 | if (options?.allowReserve !== true && remaining < snapshot.limit * GRAPHQL_RESERVE_RATIO) { | |
| 98 | return [snapshot.resetAtMs, current] as const; | |
| 99 | } | |
| 100 | const next = new Map(current); | |
| 101 | next.set(key, { ...snapshot, remaining }); | |
| 102 | return [null, next] as const; | |
| 103 | }); | |
| 104 | if (retryAt !== null) { | |
| 105 | return yield* new SourceControlRateLimit.SourceControlRateLimitPausedError({ | |
| 106 | provider: "github", | |
| 107 | host: hostKey(host), | |
| 108 | retryAt, | |
| 109 | }); | |
| 110 | } | |
| 111 | return withRateLimit(document); | |
| 112 | }, | |
| 113 | ); | |
| 114 | ||
| 115 | const observe: GitHubGraphQlBudget["Service"]["observe"] = Effect.fn( | |
| 116 | "GitHubGraphQlBudget.observe", | |
| 117 | )(function* (host, raw) { | |
| 118 | const snapshot = snapshotFrom(raw); | |
| 119 | if (snapshot === null) return; | |
| 120 | yield* Ref.update(snapshots, (current) => { | |
| 121 | const key = hostKey(host); | |
| 122 | const previous = current.get(key); | |
| 123 | // Concurrent reads can finish out of order. Quota only falls within one reset window, and | |
| 124 | // an answer from an older window must not replace the current one. | |
| 125 | if ( | |
| 126 | previous !== undefined && | |
| 127 | (snapshot.resetAtMs < previous.resetAtMs || | |
| 128 | (snapshot.resetAtMs === previous.resetAtMs && snapshot.remaining >= previous.remaining)) | |
| 129 | ) { | |
| 130 | return current; | |
| 131 | } | |
| 132 | const next = new Map(current); | |
| 133 | next.set(key, snapshot); | |
| 134 | return next; | |
| 135 | }); | |
| 136 | }); | |
| 137 | ||
| 138 | return GitHubGraphQlBudget.of({ query, observe }); | |
| 139 | }); | |
| 140 | ||
| 141 | export const layer = Layer.effect(GitHubGraphQlBudget, make); |
apps/server/src/vcs/VcsProcess.test.ts+18| ... | ... | @@ -165,6 +165,24 @@ describe("VcsProcess.run", () => { |
| 165 | 165 | }).pipe(provideLive), |
| 166 | 166 | ); |
| 167 | 167 | |
| 168 | it.effect("classifies HTTP 429 responses as rate limits", () => | |
| 169 | Effect.gen(function* () { | |
| 170 | const providerStderr = "HTTP 429: Too Many Requests. request-id=secret-value"; | |
| 171 | const error = yield* run({ | |
| 172 | operation: "test.rate-limit", | |
| 173 | command: "node", | |
| 174 | args: ["-e", "process.stderr.write(process.argv[1]); process.exit(1)", providerStderr], | |
| 175 | cwd: process.cwd(), | |
| 176 | }).pipe(Effect.flip); | |
| 177 | ||
| 178 | expect(error).toMatchObject({ | |
| 179 | detail: "API rate limit exceeded.", | |
| 180 | failureKind: "rate-limited", | |
| 181 | }); | |
| 182 | expect(error.message).not.toContain(providerStderr); | |
| 183 | }).pipe(provideLive), | |
| 184 | ); | |
| 185 | ||
| 168 | 186 | it.effect("retains spawn causes without exposing process arguments in the error message", () => |
| 169 | 187 | Effect.gen(function* () { |
| 170 | 188 | const secretArgument = "--token=super-secret-token"; |
apps/server/src/vcs/VcsProcess.ts+3-1| ... | ... | @@ -72,7 +72,9 @@ const classifyNonZeroExit = (command: string, stderr: string): VcsProcessExitFai |
| 72 | 72 | if ( |
| 73 | 73 | normalized.includes("api rate limit") || |
| 74 | 74 | normalized.includes("rate limit exceeded") || |
| 75 | normalized.includes("secondary rate limit") | |
| 75 | normalized.includes("secondary rate limit") || | |
| 76 | normalized.includes("too many requests") || | |
| 77 | normalized.includes("http 429") | |
| 76 | 78 | ) { |
| 77 | 79 | return "rate-limited"; |
| 78 | 80 | } |
apps/server/src/ws.ts+8| ... | ... | @@ -1659,6 +1659,14 @@ const makeWsRpcLayer = ( |
| 1659 | 1659 | observeRpcEffect(WS_METHODS.pullRequestsActivity, pullRequests.activity(input), { |
| 1660 | 1660 | "rpc.aggregate": "pull-requests", |
| 1661 | 1661 | }), |
| 1662 | [WS_METHODS.pullRequestsThreadComments]: (input) => | |
| 1663 | observeRpcEffect( | |
| 1664 | WS_METHODS.pullRequestsThreadComments, | |
| 1665 | pullRequests.threadComments(input), | |
| 1666 | { | |
| 1667 | "rpc.aggregate": "pull-requests", | |
| 1668 | }, | |
| 1669 | ), | |
| 1662 | 1670 | [WS_METHODS.pullRequestsDiffFileContents]: (input) => |
| 1663 | 1671 | observeRpcEffect( |
| 1664 | 1672 | WS_METHODS.pullRequestsDiffFileContents, |
apps/web/src/components/pullRequest/PullRequestCodeTab.tsx+19| ... | ... | @@ -8,6 +8,7 @@ import type { |
| 8 | 8 | PullRequestRef, |
| 9 | 9 | PullRequestReviewPosition, |
| 10 | 10 | PullRequestReviewThread, |
| 11 | PullRequestThreadCommentsResult, | |
| 11 | 12 | } from "@t3tools/contracts"; |
| 12 | 13 | import { |
| 13 | 14 | ChevronDownIcon, |
| ... | ... | @@ -336,6 +337,9 @@ export function PullRequestCodeTab({ |
| 336 | 337 | const updateComment = useAtomCommand(pullRequestEnvironment.updateComment, { |
| 337 | 338 | reportFailure: false, |
| 338 | 339 | }); |
| 340 | const loadThreadComments = useAtomCommand(pullRequestEnvironment.threadComments, { | |
| 341 | reportFailure: false, | |
| 342 | }); | |
| 339 | 343 | const getDiffFileContents = useAtomCommand(pullRequestEnvironment.diffFileContents); |
| 340 | 344 | const loadDiffFiles = useMemo( |
| 341 | 345 | () => |
| ... | ... | @@ -798,6 +802,20 @@ export function PullRequestCodeTab({ |
| 798 | 802 | fixPending={pendingFinding === pullRequestFindingKey({ kind: "thread", thread })} |
| 799 | 803 | fixLabel={fixFindingLabel} |
| 800 | 804 | {...(onFixFinding ? { onFix: () => onFixFinding({ kind: "thread", thread }) } : {})} |
| 805 | onLoadMore={async (cursor): Promise<PullRequestThreadCommentsResult | null> => { | |
| 806 | const result = await loadThreadComments({ | |
| 807 | environmentId, | |
| 808 | input: { ...reference, threadId: thread.id, cursor }, | |
| 809 | }); | |
| 810 | if (result._tag === "Failure") { | |
| 811 | toastManager.add({ | |
| 812 | type: "error", | |
| 813 | title: "More comments could not be loaded", | |
| 814 | }); | |
| 815 | return null; | |
| 816 | } | |
| 817 | return result.value; | |
| 818 | }} | |
| 801 | 819 | onReply={(body) => |
| 802 | 820 | runThreadCommand("Reply could not be posted", () => |
| 803 | 821 | replyToThread({ |
| ... | ... | @@ -833,6 +851,7 @@ export function PullRequestCodeTab({ |
| 833 | 851 | detail, |
| 834 | 852 | environmentId, |
| 835 | 853 | fixFindingLabel, |
| 854 | loadThreadComments, | |
| 836 | 855 | onRefresh, |
| 837 | 856 | onFixFinding, |
| 838 | 857 | pendingFinding, |
apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx+17-5| ... | ... | @@ -111,6 +111,7 @@ import { |
| 111 | 111 | readableFailure, |
| 112 | 112 | resolveBaseFreshness, |
| 113 | 113 | type PullRequestFinding, |
| 114 | shouldRefreshPullRequestActivity, | |
| 114 | 115 | } from "./pullRequestDetail.logic"; |
| 115 | 116 | import { canEditPullRequestChangeRequest } from "./pullRequestEditing.logic"; |
| 116 | 117 | import { |
| ... | ... | @@ -511,6 +512,17 @@ export function PullRequestDetailPanel({ |
| 511 | 512 | detailQuery.refresh(); |
| 512 | 513 | activityQuery.refresh(); |
| 513 | 514 | }, [activityQuery.refresh, detailQuery.refresh]); |
| 515 | const activityRevision = useRef<{ readonly key: string; readonly updatedAt: string } | null>( | |
| 516 | null, | |
| 517 | ); | |
| 518 | useEffect(() => { | |
| 519 | if (!coreDetail) return; | |
| 520 | const next = { key: pullRequestKey, updatedAt: coreDetail.updatedAt }; | |
| 521 | if (shouldRefreshPullRequestActivity(activityRevision.current, next)) { | |
| 522 | activityQuery.refresh(); | |
| 523 | } | |
| 524 | activityRevision.current = next; | |
| 525 | }, [activityQuery.refresh, coreDetail, pullRequestKey]); | |
| 514 | 526 | useEffect(() => { |
| 515 | 527 | if (!detail) return; |
| 516 | 528 | onStateChange?.({ |
| ... | ... | @@ -521,11 +533,11 @@ export function PullRequestDetailPanel({ |
| 521 | 533 | isDraft: detail.isDraft, |
| 522 | 534 | }); |
| 523 | 535 | }, [detail, onStateChange]); |
| 524 | // A pull request changes while it is open in front of somebody — a push lands, a check | |
| 525 | // finishes, a review arrives — so the panel reads it again on the way back to the window and | |
| 526 | // while a reader sits on it. Keyed by the pull request rather than by the panel, because this | |
| 527 | // one panel shows a different pull request every time it is opened. | |
| 528 | useLiveRefresh(refreshDetail, { | |
| 536 | // Core detail is cheap enough to re-read while this stays open. Activity is heavier, so the | |
| 537 | // revision effect above reads it only after this same pull request reports a change. Keyed by | |
| 538 | // the pull request rather than by the panel, because this one panel shows a different pull | |
| 539 | // request every time it is opened. | |
| 540 | useLiveRefresh(detailQuery.refresh, { | |
| 529 | 541 | key: `pull-request:${reference.projectId}:${reference.repository}#${reference.number}`, |
| 530 | 542 | }); |
| 531 | 543 | // The button, on the other hand, goes around the server's cache rather than through it: it is |
apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx+72-4| ... | ... | @@ -6,6 +6,7 @@ import type { |
| 6 | 6 | EnvironmentId, |
| 7 | 7 | PullRequestRef, |
| 8 | 8 | PullRequestReviewThread, |
| 9 | PullRequestThreadCommentsResult, | |
| 9 | 10 | PullRequestThreadComment, |
| 10 | 11 | } from "@t3tools/contracts"; |
| 11 | 12 | import { |
| ... | ... | @@ -24,6 +25,10 @@ import { cn } from "~/lib/utils"; |
| 24 | 25 | import { Button } from "../ui/button"; |
| 25 | 26 | import { Textarea } from "../ui/textarea"; |
| 26 | 27 | import { isCommentSubmitShortcut } from "../diffs/commentSubmitShortcut"; |
| 28 | import { | |
| 29 | editPullRequestThreadComment, | |
| 30 | mergePullRequestThreadComments, | |
| 31 | } from "./pullRequestDetail.logic"; | |
| 27 | 32 | import { PullRequestActorLabel } from "./pullRequestPresentation"; |
| 28 | 33 | import { PullRequestMarkdown } from "./PullRequestMarkdown"; |
| 29 | 34 | import { PullRequestMarkdownEditor } from "./PullRequestMarkdownEditor"; |
| ... | ... | @@ -98,6 +103,7 @@ export function ReviewThreadCard({ |
| 98 | 103 | fixLabel = "Fix in a thread", |
| 99 | 104 | onFix, |
| 100 | 105 | onReply, |
| 106 | onLoadMore, | |
| 101 | 107 | canEditComment, |
| 102 | 108 | onEditComment, |
| 103 | 109 | onToggleResolved, |
| ... | ... | @@ -118,6 +124,8 @@ export function ReviewThreadCard({ |
| 118 | 124 | onFix?: () => void; |
| 119 | 125 | /** Resolves to whether the host took it, so a reply that failed keeps the words it was given. */ |
| 120 | 126 | onReply: (body: string) => Promise<boolean>; |
| 127 | /** Reads one more page only after the reader asks for it. */ | |
| 128 | onLoadMore: (cursor: string) => Promise<PullRequestThreadCommentsResult | null>; | |
| 121 | 129 | /** Whether this reader wrote this remark, which is what rewriting one takes. */ |
| 122 | 130 | canEditComment: (comment: PullRequestThreadComment) => boolean; |
| 123 | 131 | /** Resolves to whether the host took it, like `onReply`. */ |
| ... | ... | @@ -132,13 +140,32 @@ export function ReviewThreadCard({ |
| 132 | 140 | const [editingId, setEditingId] = useState<string | null>(null); |
| 133 | 141 | const [savingEdit, setSavingEdit] = useState(false); |
| 134 | 142 | const sendingRef = useRef(false); |
| 143 | const [loadedPage, setLoadedPage] = useState< | |
| 144 | (PullRequestThreadCommentsResult & { readonly threadId: string }) | null | |
| 145 | >(null); | |
| 146 | const [loadingMore, setLoadingMore] = useState(false); | |
| 147 | const currentPage = loadedPage?.threadId === thread.id ? loadedPage : null; | |
| 148 | const comments = mergePullRequestThreadComments(thread.comments, currentPage?.comments ?? []); | |
| 149 | const nextCommentsCursor = | |
| 150 | currentPage === null ? (thread.nextCommentsCursor ?? null) : currentPage.nextCursor; | |
| 151 | const commentCount = thread.commentCount ?? comments.length; | |
| 135 | 152 | |
| 136 | 153 | const saveEdit = async (commentId: string, body: string) => { |
| 137 | 154 | if (savingEdit) return; |
| 138 | 155 | setSavingEdit(true); |
| 139 | 156 | const saved = await onEditComment(commentId, body); |
| 140 | 157 | setSavingEdit(false); |
| 141 | if (saved) setEditingId(null); | |
| 158 | if (saved) { | |
| 159 | setLoadedPage((previous) => | |
| 160 | previous?.threadId === thread.id | |
| 161 | ? { | |
| 162 | ...previous, | |
| 163 | comments: editPullRequestThreadComment(previous.comments, commentId, body), | |
| 164 | } | |
| 165 | : previous, | |
| 166 | ); | |
| 167 | setEditingId(null); | |
| 168 | } | |
| 142 | 169 | }; |
| 143 | 170 | |
| 144 | 171 | const send = async () => { |
| ... | ... | @@ -149,6 +176,16 @@ export function ReviewThreadCard({ |
| 149 | 176 | // empty box, and the words have to be written again. |
| 150 | 177 | try { |
| 151 | 178 | if (await onReply(trimmed)) { |
| 179 | // The mutation returns no comment. Keep what the reader loaded and reopen its cursor so | |
| 180 | // the new reply remains reachable without spending requests until they ask to load it. | |
| 181 | setLoadedPage((previous) => | |
| 182 | previous?.threadId === thread.id | |
| 183 | ? { | |
| 184 | ...previous, | |
| 185 | nextCursor: previous.nextCursor ?? thread.nextCommentsCursor ?? null, | |
| 186 | } | |
| 187 | : previous, | |
| 188 | ); | |
| 152 | 189 | setReply(""); |
| 153 | 190 | setReplying(false); |
| 154 | 191 | } |
| ... | ... | @@ -156,6 +193,24 @@ export function ReviewThreadCard({ |
| 156 | 193 | sendingRef.current = false; |
| 157 | 194 | } |
| 158 | 195 | }; |
| 196 | const loadMore = async () => { | |
| 197 | if (nextCommentsCursor === null || loadingMore) return; | |
| 198 | setLoadingMore(true); | |
| 199 | try { | |
| 200 | const page = await onLoadMore(nextCommentsCursor); | |
| 201 | if (page === null) return; | |
| 202 | setLoadedPage((previous) => ({ | |
| 203 | threadId: thread.id, | |
| 204 | comments: mergePullRequestThreadComments( | |
| 205 | previous?.threadId === thread.id ? previous.comments : [], | |
| 206 | page.comments, | |
| 207 | ), | |
| 208 | nextCursor: page.nextCursor, | |
| 209 | })); | |
| 210 | } finally { | |
| 211 | setLoadingMore(false); | |
| 212 | } | |
| 213 | }; | |
| 159 | 214 | |
| 160 | 215 | return ( |
| 161 | 216 | <div |
| ... | ... | @@ -175,8 +230,8 @@ export function ReviewThreadCard({ |
| 175 | 230 | aria-expanded={expanded} |
| 176 | 231 | onClick={() => setExpanded((current) => !current)} |
| 177 | 232 | > |
| 178 | {thread.isResolved ? "Resolved" : "Open"} · {thread.comments.length}{" "} | |
| 179 | {thread.comments.length === 1 ? "comment" : "comments"} | |
| 233 | {thread.isResolved ? "Resolved" : "Open"} · {commentCount}{" "} | |
| 234 | {commentCount === 1 ? "comment" : "comments"} | |
| 180 | 235 | </button> |
| 181 | 236 | {thread.isOutdated ? <span>outdated</span> : null} |
| 182 | 237 | {onFix ? ( |
| ... | ... | @@ -207,7 +262,7 @@ export function ReviewThreadCard({ |
| 207 | 262 | {expanded ? ( |
| 208 | 263 | <> |
| 209 | 264 | <div className="mt-2 space-y-3"> |
| 210 | {thread.comments.map((comment) => ( | |
| 265 | {comments.map((comment) => ( | |
| 211 | 266 | <article key={comment.id} className="group min-w-0"> |
| 212 | 267 | <div className="flex items-center gap-2 text-xs text-muted-foreground"> |
| 213 | 268 | <PullRequestActorLabel actor={comment.author} className="text-foreground" /> |
| ... | ... | @@ -255,6 +310,19 @@ export function ReviewThreadCard({ |
| 255 | 310 | </article> |
| 256 | 311 | ))} |
| 257 | 312 | </div> |
| 313 | {nextCommentsCursor !== null ? ( | |
| 314 | <div className="mt-2"> | |
| 315 | <Button | |
| 316 | size="xs" | |
| 317 | variant="ghost" | |
| 318 | className="px-1" | |
| 319 | disabled={loadingMore} | |
| 320 | onClick={() => void loadMore()} | |
| 321 | > | |
| 322 | {loadingMore ? "Loading..." : "Load more comments"} | |
| 323 | </Button> | |
| 324 | </div> | |
| 325 | ) : null} | |
| 258 | 326 | |
| 259 | 327 | {canReply ? ( |
| 260 | 328 | replying ? ( |
apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts+62| ... | ... | @@ -17,6 +17,7 @@ import { |
| 17 | 17 | handoffPrompt, |
| 18 | 18 | handoffReviewComments, |
| 19 | 19 | isThreadOwnPullRequest, |
| 20 | mergePullRequestThreadComments, | |
| 20 | 21 | orderPullRequestComments, |
| 21 | 22 | pullRequestActionMenuHasGroup, |
| 22 | 23 | pullRequestActionNeedsHostRefresh, |
| ... | ... | @@ -24,9 +25,11 @@ import { |
| 24 | 25 | pullRequestFindingKey, |
| 25 | 26 | pullRequestHandoffLabels, |
| 26 | 27 | readableFailure, |
| 28 | shouldRefreshPullRequestActivity, | |
| 27 | 29 | resolveBaseFreshness, |
| 28 | 30 | buildPullRequestTimeline, |
| 29 | 31 | describePullRequestState, |
| 32 | editPullRequestThreadComment, | |
| 30 | 33 | } from "./pullRequestDetail.logic"; |
| 31 | 34 | import type { ReviewCommentContext } from "~/reviewCommentContext"; |
| 32 | 35 | |
| ... | ... | @@ -55,6 +58,65 @@ const TIMELINE_SOURCE: Pick< |
| 55 | 58 | closedAt: null, |
| 56 | 59 | }; |
| 57 | 60 | |
| 61 | describe("pull request activity refresh", () => { | |
| 62 | const first = { | |
| 63 | key: "project:acme/web#7", | |
| 64 | updatedAt: "2026-08-13T13:00:00Z", | |
| 65 | }; | |
| 66 | ||
| 67 | it("refreshes activity only after the same pull request changes", () => { | |
| 68 | expect( | |
| 69 | shouldRefreshPullRequestActivity(first, { | |
| 70 | ...first, | |
| 71 | updatedAt: "2026-08-13T13:01:00Z", | |
| 72 | }), | |
| 73 | ).toBe(true); | |
| 74 | }); | |
| 75 | ||
| 76 | it("does not duplicate the first activity read or carry a revision across pull requests", () => { | |
| 77 | expect(shouldRefreshPullRequestActivity(null, first)).toBe(false); | |
| 78 | expect(shouldRefreshPullRequestActivity(first, first)).toBe(false); | |
| 79 | expect( | |
| 80 | shouldRefreshPullRequestActivity(first, { | |
| 81 | key: "project:acme/web#8", | |
| 82 | updatedAt: "2026-08-13T13:01:00Z", | |
| 83 | }), | |
| 84 | ).toBe(false); | |
| 85 | }); | |
| 86 | }); | |
| 87 | describe("review thread comment pages", () => { | |
| 88 | it("appends new comments once and keeps refreshed base comments", () => { | |
| 89 | expect( | |
| 90 | mergePullRequestThreadComments( | |
| 91 | [ | |
| 92 | { id: "c1", body: "refreshed" }, | |
| 93 | { id: "c2", body: "already in base" }, | |
| 94 | ], | |
| 95 | [ | |
| 96 | { id: "c2", body: "stale page copy" }, | |
| 97 | { id: "c3", body: "next page" }, | |
| 98 | ], | |
| 99 | ), | |
| 100 | ).toEqual([ | |
| 101 | { id: "c1", body: "refreshed" }, | |
| 102 | { id: "c2", body: "already in base" }, | |
| 103 | { id: "c3", body: "next page" }, | |
| 104 | ]); | |
| 105 | }); | |
| 106 | ||
| 107 | it("keeps a loaded comment after its body is edited", () => { | |
| 108 | const loaded = [ | |
| 109 | { id: "c2", body: "old body" }, | |
| 110 | { id: "c3", body: "another loaded comment" }, | |
| 111 | ]; | |
| 112 | ||
| 113 | expect(editPullRequestThreadComment(loaded, "c2", "saved body")).toEqual([ | |
| 114 | { id: "c2", body: "saved body" }, | |
| 115 | { id: "c3", body: "another loaded comment" }, | |
| 116 | ]); | |
| 117 | }); | |
| 118 | }); | |
| 119 | ||
| 58 | 120 | describe("pull request action menu", () => { |
| 59 | 121 | it("keeps the group divider when auto-merge is the only action", () => { |
| 60 | 122 | expect(pullRequestActionMenuHasGroup(false, true, false)).toBe(true); |
apps/web/src/components/pullRequest/pullRequestDetail.logic.ts+29| ... | ... | @@ -14,6 +14,35 @@ import type { |
| 14 | 14 | |
| 15 | 15 | import { inferReviewCommentFenceLanguage, type ReviewCommentContext } from "~/reviewCommentContext"; |
| 16 | 16 | |
| 17 | /** Activity changes only when the same host resource reports a newer revision. */ | |
| 18 | export function shouldRefreshPullRequestActivity( | |
| 19 | previous: { readonly key: string; readonly updatedAt: string } | null, | |
| 20 | next: { readonly key: string; readonly updatedAt: string }, | |
| 21 | ): boolean { | |
| 22 | return previous !== null && previous.key === next.key && previous.updatedAt !== next.updatedAt; | |
| 23 | } | |
| 24 | /** Appends fetched pages without replacing fresher comments already in the activity response. */ | |
| 25 | export function mergePullRequestThreadComments<T extends { readonly id: string }>( | |
| 26 | base: ReadonlyArray<T>, | |
| 27 | loaded: ReadonlyArray<T>, | |
| 28 | ): ReadonlyArray<T> { | |
| 29 | const seen = new Set(base.map((comment) => comment.id)); | |
| 30 | return [ | |
| 31 | ...base, | |
| 32 | ...loaded.filter((comment) => { | |
| 33 | if (seen.has(comment.id)) return false; | |
| 34 | seen.add(comment.id); | |
| 35 | return true; | |
| 36 | }), | |
| 37 | ]; | |
| 38 | } | |
| 39 | ||
| 40 | export function editPullRequestThreadComment< | |
| 41 | T extends { readonly id: string; readonly body: string }, | |
| 42 | >(comments: ReadonlyArray<T>, commentId: string, body: string): ReadonlyArray<T> { | |
| 43 | return comments.map((comment) => (comment.id === commentId ? { ...comment, body } : comment)); | |
| 44 | } | |
| 45 | ||
| 17 | 46 | /** |
| 18 | 47 | * Whether the pull request on a right-panel surface is the thread's own one. Repository and |
| 19 | 48 | * number are not enough: one environment can hold two checkouts of the same repository under |
apps/web/src/hooks/useLiveRefresh.test.ts+11-1| ... | ... | @@ -9,6 +9,12 @@ import { |
| 9 | 9 | shouldRefreshOnInterval, |
| 10 | 10 | } from "./useLiveRefresh"; |
| 11 | 11 | |
| 12 | describe("live refresh cadence", () => { | |
| 13 | it("waits five minutes between automatic host reads", () => { | |
| 14 | expect(LIVE_REFRESH_INTERVAL_MS).toBe(5 * 60_000); | |
| 15 | }); | |
| 16 | }); | |
| 17 | ||
| 12 | 18 | describe("shouldLiveRefresh", () => { |
| 13 | 19 | const at = (now: number, lastRefreshedAt: number, visible = true) => |
| 14 | 20 | shouldLiveRefresh({ visible, now, lastRefreshedAt }); |
| ... | ... | @@ -79,12 +85,16 @@ describe("shouldRefreshOnInterval", () => { |
| 79 | 85 | expect(tick(LIVE_REFRESH_INTERVAL_MS, LIVE_REFRESH_INTERVAL_MS - 1_000)).toBe(true); |
| 80 | 86 | }); |
| 81 | 87 | |
| 88 | it("reads on the first interval after an untouched mount", () => { | |
| 89 | expect(tick(LIVE_REFRESH_INTERVAL_MS + 1_000, 0)).toBe(true); | |
| 90 | }); | |
| 91 | ||
| 82 | 92 | it("stops reading for a window left showing on a desk nobody is at", () => { |
| 83 | 93 | expect(tick(LIVE_REFRESH_IDLE_AFTER_MS + 60_000, 0)).toBe(false); |
| 84 | 94 | }); |
| 85 | 95 | |
| 86 | 96 | it("starts reading again once the reader touches the window", () => { |
| 87 | 97 | const away = LIVE_REFRESH_IDLE_AFTER_MS + 60_000; |
| 88 | expect(tick(away + LIVE_REFRESH_INTERVAL_MS, away)).toBe(true); | |
| 98 | expect(tick(away + LIVE_REFRESH_MIN_INTERVAL_MS, away)).toBe(true); | |
| 89 | 99 | }); |
| 90 | 100 | }); |
apps/web/src/hooks/useLiveRefresh.ts+5-4| ... | ... | @@ -18,16 +18,17 @@ import { useEffect, useId, useRef } from "react"; |
| 18 | 18 | /** Long enough that alt-tabbing through windows does not become a request per tab stop. */ |
| 19 | 19 | export const LIVE_REFRESH_MIN_INTERVAL_MS = 10_000; |
| 20 | 20 | |
| 21 | /** Slow enough to be cheap on the host, quick enough that a reader is not reading last minute. */ | |
| 22 | export const LIVE_REFRESH_INTERVAL_MS = 60_000; | |
| 21 | /** Slow enough to preserve host quota while still updating a view left open. */ | |
| 22 | export const LIVE_REFRESH_INTERVAL_MS = 5 * 60_000; | |
| 23 | 23 | |
| 24 | 24 | /** |
| 25 | * How long a showing window goes untouched before it stops reading. A window left open on a | |
| 25 | * How long a showing window goes untouched before it stops reading. This leaves one minute after | |
| 26 | * the five-minute interval for the first timer tick to run. A window left open on a | |
| 26 | 27 | * monitor nobody is sitting at is showing in every sense the browser knows about, and polling it |
| 27 | 28 | * until morning spends a night of somebody's rate limit on an answer nobody read. Their next |
| 28 | 29 | * click, key or scroll starts the interval up again. |
| 29 | 30 | */ |
| 30 | export const LIVE_REFRESH_IDLE_AFTER_MS = 5 * 60_000; | |
| 31 | export const LIVE_REFRESH_IDLE_AFTER_MS = 6 * 60_000; | |
| 31 | 32 | |
| 32 | 33 | /** |
| 33 | 34 | * Whether a view should read again now. Separate from the hook because the rule is the whole of |
apps/web/src/routes/_chat.pull-requests.tsx-1| ... | ... | @@ -972,7 +972,6 @@ function PullRequestsRouteView() { |
| 972 | 972 | useLiveRefresh( |
| 973 | 973 | () => { |
| 974 | 974 | refreshList(); |
| 975 | baselineQuery.refresh(); | |
| 976 | 975 | authoredQuery.refresh(); |
| 977 | 976 | reviewingQuery.refresh(); |
| 978 | 977 | }, |
packages/client-runtime/src/state/pullRequests.ts+10| ... | ... | @@ -61,6 +61,16 @@ export function createPullRequestEnvironmentAtoms<R, E>( |
| 61 | 61 | tag: WS_METHODS.pullRequestsActivity, |
| 62 | 62 | staleTimeMs: 15_000, |
| 63 | 63 | }), |
| 64 | threadComments: createEnvironmentRpcCommand(runtime, { | |
| 65 | label: "environment-data:pull-requests:thread-comments", | |
| 66 | tag: WS_METHODS.pullRequestsThreadComments, | |
| 67 | scheduler: commandScheduler, | |
| 68 | concurrency: { | |
| 69 | mode: "singleFlight", | |
| 70 | key: ({ environmentId, input }) => | |
| 71 | JSON.stringify([environmentId, input.threadId, input.cursor]), | |
| 72 | }, | |
| 73 | }), | |
| 64 | 74 | diff: createEnvironmentQueryAtomFamily(runtime, { |
| 65 | 75 | label: "environment-data:pull-requests:diff", |
| 66 | 76 | staleTimeMs: 60_000, |
packages/contracts/src/pullRequest.ts+17| ... | ... | @@ -241,6 +241,10 @@ export const PullRequestReviewThread = Schema.Struct({ |
| 241 | 241 | */ |
| 242 | 242 | isOutdated: Schema.Boolean, |
| 243 | 243 | comments: Schema.Array(PullRequestThreadComment), |
| 244 | /** Host-reported total, when this thread was read in pages. */ | |
| 245 | commentCount: Schema.optional(NonNegativeInt), | |
| 246 | /** Opaque cursor for the next comment page. Absent once this thread is whole. */ | |
| 247 | nextCommentsCursor: Schema.optional(TrimmedNonEmptyString), | |
| 244 | 248 | }); |
| 245 | 249 | export type PullRequestReviewThread = typeof PullRequestReviewThread.Type; |
| 246 | 250 | |
| ... | ... | @@ -904,6 +908,19 @@ export const PullRequestSubmitReviewInput = Schema.Struct({ |
| 904 | 908 | }); |
| 905 | 909 | export type PullRequestSubmitReviewInput = typeof PullRequestSubmitReviewInput.Type; |
| 906 | 910 | |
| 911 | export const PullRequestThreadCommentsInput = Schema.Struct({ | |
| 912 | ...PullRequestRef.fields, | |
| 913 | threadId: TrimmedNonEmptyString, | |
| 914 | cursor: TrimmedNonEmptyString, | |
| 915 | }); | |
| 916 | export type PullRequestThreadCommentsInput = typeof PullRequestThreadCommentsInput.Type; | |
| 917 | ||
| 918 | export const PullRequestThreadCommentsResult = Schema.Struct({ | |
| 919 | comments: Schema.Array(PullRequestThreadComment), | |
| 920 | nextCursor: Schema.NullOr(TrimmedNonEmptyString), | |
| 921 | }); | |
| 922 | export type PullRequestThreadCommentsResult = typeof PullRequestThreadCommentsResult.Type; | |
| 923 | ||
| 907 | 924 | export const PullRequestThreadReplyInput = Schema.Struct({ |
| 908 | 925 | ...PullRequestRef.fields, |
| 909 | 926 | threadId: TrimmedNonEmptyString, |
packages/contracts/src/rpc.ts+10| ... | ... | @@ -86,6 +86,8 @@ import { |
| 86 | 86 | PullRequestReviewerCandidateList, |
| 87 | 87 | PullRequestReviewerRequestInput, |
| 88 | 88 | PullRequestSubmitReviewInput, |
| 89 | PullRequestThreadCommentsInput, | |
| 90 | PullRequestThreadCommentsResult, | |
| 89 | 91 | PullRequestThreadReplyInput, |
| 90 | 92 | PullRequestThreadResolutionInput, |
| 91 | 93 | PullRequestUnavailableError, |
| ... | ... | @@ -281,6 +283,7 @@ export const WS_METHODS = { |
| 281 | 283 | pullRequestsListStats: "pullRequests.listStats", |
| 282 | 284 | pullRequestsDetail: "pullRequests.detail", |
| 283 | 285 | pullRequestsActivity: "pullRequests.activity", |
| 286 | pullRequestsThreadComments: "pullRequests.threadComments", | |
| 284 | 287 | pullRequestsDiffFileContents: "pullRequests.diffFileContents", |
| 285 | 288 | pullRequestsRunAction: "pullRequests.runAction", |
| 286 | 289 | pullRequestsUpdate: "pullRequests.update", |
| ... | ... | @@ -502,6 +505,12 @@ export const WsPullRequestsActivityRpc = Rpc.make(WS_METHODS.pullRequestsActivit |
| 502 | 505 | error: PullRequestRpcError, |
| 503 | 506 | }); |
| 504 | 507 | |
| 508 | export const WsPullRequestsThreadCommentsRpc = Rpc.make(WS_METHODS.pullRequestsThreadComments, { | |
| 509 | payload: PullRequestThreadCommentsInput, | |
| 510 | success: PullRequestThreadCommentsResult, | |
| 511 | error: PullRequestRpcError, | |
| 512 | }); | |
| 513 | ||
| 505 | 514 | export const WsPullRequestsDiffFileContentsRpc = Rpc.make(WS_METHODS.pullRequestsDiffFileContents, { |
| 506 | 515 | payload: PullRequestDiffFileContentsInput, |
| 507 | 516 | success: PullRequestDiffFileContentsResult, |
| ... | ... | @@ -1001,6 +1010,7 @@ export const WsRpcGroup = RpcGroup.make( |
| 1001 | 1010 | WsPullRequestsListStatsRpc, |
| 1002 | 1011 | WsPullRequestsDetailRpc, |
| 1003 | 1012 | WsPullRequestsActivityRpc, |
| 1013 | WsPullRequestsThreadCommentsRpc, | |
| 1004 | 1014 | WsPullRequestsDiffFileContentsRpc, |
| 1005 | 1015 | WsPullRequestsRunActionRpc, |
| 1006 | 1016 | WsPullRequestsUpdateRpc, |