diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts index 8ad71188e187..c8ac87257422 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts @@ -801,8 +801,9 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { yield* ask(); const again = yield* ask(); - assert.strictEqual(mockedExecute.mock.calls.length, 5); - // The second read goes straight to the pushes, and still answers with the head's blob. + assert.strictEqual(mockedExecute.mock.calls.length, 4); + // The second read goes straight to the pushes, finds the same push's changes already held, + // and still answers with the head's blob. expect(argsOfCall(3)).toContain("pullRequestIterations"); expect([...again.revisions]).toEqual([["README.md", "8f80"]]); }), @@ -1148,7 +1149,7 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }), ); - it.effect("stops following pages when one of them does not move the cursor on", () => + it.effect("retries truncated changes without a push and retains complete results", () => Effect.gen(function* () { mockedExecute .mockReturnValueOnce( @@ -1201,17 +1202,35 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }), ), ), - ); + ) + .mockReturnValueOnce(Effect.succeed(output(json(oneIteration)))) + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + changeEntries: [ + { changeType: "edit", item: { path: "/a.ts", objectId: "8f80" } }, + { changeType: "edit", item: { path: "/b.ts", objectId: "0ca4" } }, + { changeType: "add", item: { path: "/unlisted.ts", objectId: "cafe" } }, + ], + }), + ), + ), + ) + .mockReturnValueOnce(Effect.succeed(output(json(oneIteration)))); const provider = yield* AzureDevOpsPullRequestProvider.make; - assert.isDefined(provider.getFileRevisions); + const read = provider.getFileRevisions; + assert.isDefined(read); - const answer = yield* provider.getFileRevisions({ - cwd: "/w", - repository: "web", - host: "dev.azure.com", - number: 42, - paths: ["a.ts", "b.ts", "unlisted.ts"], - }); + const ask = () => + read({ + cwd: "/w", + repository: "web", + host: "dev.azure.com", + number: 42, + paths: ["a.ts", "b.ts", "unlisted.ts"], + }); + const answer = yield* ask(); // Four reads and no more: a page pointing at where it already is would be read forever. assert.strictEqual(mockedExecute.mock.calls.length, 4); @@ -1221,6 +1240,22 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { ["a.ts", "8f80"], ["b.ts", "0ca4"], ]); + + const recovered = yield* ask(); + assert.strictEqual(mockedExecute.mock.calls.length, 6); + expect(argsOfCall(4)).toContain("pullRequestIterations"); + expect(argsOfCall(5)).toContain("pullRequestIterationChanges"); + expect(argsOfCall(5)).toContain("iterationId=1"); + expect([...recovered.revisions]).toEqual([ + ["a.ts", "8f80"], + ["b.ts", "0ca4"], + ["unlisted.ts", "cafe"], + ]); + + const cached = yield* ask(); + assert.strictEqual(mockedExecute.mock.calls.length, 7); + expect(argsOfCall(6)).toContain("pullRequestIterations"); + expect([...cached.revisions]).toEqual([...recovered.revisions]); }), ); diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index 78d31da360f5..793624a316df 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -1,5 +1,8 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; +import * as Cache from "effect/Cache"; +import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; import * as Semaphore from "effect/Semaphore"; import type { PullRequestCapabilities, PullRequestViewerPermissions } from "@t3tools/contracts"; @@ -54,6 +57,13 @@ export const MAX_DIFF_SPAWNS = 2 * DIFF_FILE_CONCURRENCY; /** How many pull requests' repository locations one provider remembers at once. */ export const LOCATION_CACHE_CAPACITY = 128; +/** + * How many pushes' change lists one provider remembers. A list never changes once its push lands, + * but can run to thousands of entries, so this holds a handful of reviews rather than a listing's + * worth. + */ +const ITERATION_CHANGES_CAPACITY = 32; + const CAPABILITIES: PullRequestCapabilities = { // Azure serves no patch of its own, so the one the Code tab reads is built here out of the // files an iteration changed and both sides of each of them. @@ -145,8 +155,29 @@ export const make = Effect.gen(function* () { // Made once with the provider, which the registry builds once, so this is the whole build's // allowance rather than one request's. const diffSpawns = yield* Semaphore.make(MAX_DIFF_SPAWNS); + // Diff slices and context expansion read the same immutable blobs. Bound both count and size. + const itemContents = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", project = "", repository = "", commit = "", path = ""] = key.split("\0"); + return diffSpawns.withPermits(1)( + cli.readItemContent({ cwd, location: { project, repository }, commit, path }), + ); + }, + { + capacity: 64, + timeToLive: (exit) => + Exit.isSuccess(exit) && exit.value.contents.length <= 128 * 1024 + ? Duration.infinity + : Duration.zero, + }, + ); const readItemContent = (input: Parameters[0]) => - diffSpawns.withPermits(1)(cli.readItemContent(input)); + Cache.get( + itemContents, + [input.cwd, input.location.project, input.location.repository, input.commit, input.path].join( + "\0", + ), + ); const fail = (operation: string) => (error: AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCliError) => @@ -173,36 +204,72 @@ export const make = Effect.gen(function* () { const EMPTY_DIFF_SLICE: ProviderDiffSlice = { patch: "", truncated: false, nextCursor: null }; /** - * Where a pull request's repository lives, which is the route every other read of it needs and - * the one thing only the pull request itself states. A pull request cannot move between + * Where a pull request's repository lives, which is the route every other read of it needs. A + * pull request read and a listing row both state it, and a pull request cannot move between * repositories, so it is remembered rather than re-read: the marks alone would otherwise pay for * a whole pull request read every time they checked whether a file had been pushed to. Least - * recently used, so a listing walking cold pull requests cannot evict the one being read. + * recently used, so the pull requests being read stay. */ const locations = new Map(); - const locationOf = (input: { readonly cwd: string; readonly number: number }) => { + const rememberLocation = (cwd: string, pullRequest: AzureDevOpsPullRequest) => { + if (pullRequest.location === null) return; + const key = `${cwd} ${pullRequest.number}`; + locations.delete(key); + if (locations.size >= LOCATION_CACHE_CAPACITY) { + const oldest = locations.keys().next().value; + if (oldest !== undefined) locations.delete(oldest); + } + locations.set(key, pullRequest.location); + }; + + const knownLocation = (input: { readonly cwd: string; readonly number: number }) => { const key = `${input.cwd} ${input.number}`; const held = locations.get(key); if (held !== undefined) { locations.delete(key); locations.set(key, held); - return Effect.succeed(held); } - return cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( - Effect.map((pullRequest) => { - const location = pullRequest.location; - if (location === null) return null; - if (locations.size >= LOCATION_CACHE_CAPACITY) { - const oldest = locations.keys().next().value; - if (oldest !== undefined) locations.delete(oldest); - } - locations.set(key, location); - return location; - }), - ); + return held; + }; + + const readPullRequest = (input: { readonly cwd: string; readonly number: number }) => + cli + .getPullRequest(input) + .pipe( + Effect.tap((pullRequest) => Effect.sync(() => rememberLocation(input.cwd, pullRequest))), + ); + + const locationOf = (input: { readonly cwd: string; readonly number: number }) => { + const held = knownLocation(input); + return held !== undefined + ? Effect.succeed(held) + : readPullRequest(input).pipe(Effect.map((pullRequest) => pullRequest.location)); }; + /** + * What one push changed, against the merge base. Never changes once the push lands, so the + * detail's file count, every slice of the diff and the viewed marks share one complete read. + * Incomplete reads are retried rather than keeping missing files for the life of the push. + */ + const iterationChanges = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", project = "", repository = "", number = "", iterationId = ""] = + key.split("\0"); + return cli.listIterationChanges({ + cwd, + location: { project, repository }, + number: Number(number), + iterationId: Number(iterationId), + }); + }, + { + capacity: ITERATION_CHANGES_CAPACITY, + timeToLive: (exit) => + Exit.isSuccess(exit) && !exit.value.truncated ? Duration.infinity : Duration.zero, + }, + ); + /** * Everything a diff read needs before it can ask for a file: where the repository lives, and * which pushes the pull request has had. A client names neither, and the iterations are read @@ -269,6 +336,28 @@ export const make = Effect.gen(function* () { * reported against the merge base rather than against the push before it, so the newest one is * the whole of the change rather than the last slice of it. */ + const changesOf = ( + input: { + readonly cwd: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly number: number; + }, + iteration: AzureDevOpsIteration, + ) => + Cache.get( + iterationChanges, + // The commits make the key immutable; the rest is what the read is addressed by. + [ + input.cwd, + input.location.project, + input.location.repository, + input.number, + iteration.id, + iteration.headCommit, + iteration.mergeBaseCommit, + ].join("\0"), + ); + const listLatestChanges = (input: { readonly cwd: string; readonly location: AzureDevOpsRepositoryLocation; @@ -281,12 +370,7 @@ export const make = Effect.gen(function* () { changes: [], truncated: false, } as AzureDevOpsPullRequestCli.AzureDevOpsIterationChanges) - : cli.listIterationChanges({ - cwd: input.cwd, - location: input.location, - number: input.number, - iterationId: latest.id, - }); + : changesOf(input, latest); }; const provider: PullRequestProviderApi = { @@ -314,6 +398,13 @@ export const make = Effect.gen(function* () { }) .pipe( Effect.mapError(fail("listChangeRequests")), + // A row states where its pull request lives, so opening one from the list needs no + // read of the pull request before its conversation or its pushes can be asked for. + Effect.tap((batch) => + Effect.sync(() => { + for (const row of batch.items) rememberLocation(input.cwd, row); + }), + ), Effect.map((batch) => ({ items: batch.items.map(toChangeRequest), truncated: batch.truncated, @@ -327,7 +418,7 @@ export const make = Effect.gen(function* () { // The polled path a linked thread's row stays live on: one `az` read, no iterations or // changes behind it, since the file count that would cost is not shown here. getChangeRequestSummary: (input) => - cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( + readPullRequest(input).pipe( Effect.mapError(fail("getChangeRequestSummary")), Effect.map((pullRequest): ProviderChangeRequestSummary => ({ number: pullRequest.number, @@ -345,27 +436,43 @@ export const make = Effect.gen(function* () { })), ), + // A hover card is the pull request read alone, where the detail it would otherwise fall back + // to reads its pushes and their changes behind it for a file count the card never shows. + getChangeRequestPreview: (input) => + readPullRequest(input).pipe( + Effect.mapError(fail("getChangeRequestPreview")), + Effect.map((pullRequest) => ({ + number: pullRequest.number, + title: pullRequest.title, + url: pullRequest.url, + author: pullRequest.author, + state: pullRequest.state, + isDraft: pullRequest.isDraft, + createdAt: pullRequest.createdAt, + })), + ), + getChangeRequest: (input) => Effect.gen(function* () { - const pullRequest = yield* cli.getPullRequest({ cwd: input.cwd, number: input.number }); - const location = pullRequest.location; // The file count is two reads past the pull request itself, and it is the only thing // riding on them, so a failure leaves it unknown rather than losing the whole detail. - const changedFiles = - location === null - ? 0 - : yield* cli.listIterations({ cwd: input.cwd, location, number: input.number }).pipe( - Effect.flatMap((iterations) => - listLatestChanges({ - cwd: input.cwd, - location, - number: input.number, - iterations, - }), - ), - Effect.map((listed) => listed.changes.length), - Effect.orElseSucceed(() => 0), - ); + const countFiles = (location: AzureDevOpsRepositoryLocation) => { + const scope = { cwd: input.cwd, location, number: input.number }; + return cli.listIterations(scope).pipe( + Effect.flatMap((iterations) => listLatestChanges({ ...scope, iterations })), + Effect.map((listed) => listed.changes.length), + Effect.orElseSucceed(() => 0), + ); + }; + // Once anything has placed the pull request, its pushes are read alongside it rather + // than after it. + const known = knownLocation(input); + const [pullRequest, knownCount] = yield* Effect.all( + [readPullRequest(input), known === undefined ? Effect.succeed(null) : countFiles(known)], + { concurrency: 2 }, + ); + const location = pullRequest.location; + const changedFiles = knownCount ?? (location === null ? 0 : yield* countFiles(location)); const detail: ProviderChangeRequestDetail = { ...toChangeRequest(pullRequest), body: pullRequest.body, @@ -385,21 +492,24 @@ export const make = Effect.gen(function* () { }).pipe(Effect.mapError(fail("getChangeRequest"))), getChangeRequestActivity: (input) => - cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( - Effect.mapError(fail("getChangeRequestActivity")), - Effect.flatMap((pullRequest) => - (pullRequest.location === null + locationOf(input).pipe( + Effect.flatMap((location) => + (location === null ? Effect.succeed({ comments: [], truncated: true }) - : cli - .listThreads({ - cwd: input.cwd, - location: pullRequest.location, - number: input.number, - }) - .pipe( - Effect.map((comments) => ({ comments, truncated: false })), - Effect.orElseSucceed(() => ({ comments: [], truncated: true })), - ) + : cli.listThreads({ cwd: input.cwd, location, number: input.number }).pipe( + Effect.map((comments) => ({ comments, truncated: false })), + // Only what is this conversation's problem. A signed-out CLI, a rate limit or no + // `az` at all is the host failing, and with a remembered location there is no pull + // request read ahead of this one left to report it. + Effect.catchTags({ + AzureDevOpsPullRequestNotFoundError: () => + Effect.succeed({ comments: [], truncated: true }), + AzureDevOpsCommandFailedError: () => + Effect.succeed({ comments: [], truncated: true }), + AzureDevOpsPullRequestReadError: () => + Effect.succeed({ comments: [], truncated: true }), + }), + ) ).pipe( Effect.map((conversation): ProviderChangeRequestActivity => ({ comments: conversation.comments, @@ -410,6 +520,7 @@ export const make = Effect.gen(function* () { })), ), ), + Effect.mapError(fail("getChangeRequestActivity")), ), // No request at all: Azure has nothing to say about the viewer that a pull request read can @@ -430,12 +541,10 @@ export const make = Effect.gen(function* () { ? scope.iterations.at(-1) : scope.iterations.find((candidate) => candidate.id === cursor.iterationId); if (iteration === undefined) return EMPTY_DIFF_SLICE; - const listed = yield* cli.listIterationChanges({ - cwd: input.cwd, - location: scope.location, - number: input.number, - iterationId: iteration.id, - }); + const listed = yield* changesOf( + { cwd: input.cwd, location: scope.location, number: input.number }, + iteration, + ); const changes = listed.changes; const sections: string[] = []; diff --git a/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.test.ts b/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.test.ts index 8d8feaba8150..5679380ffc64 100644 --- a/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.test.ts +++ b/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.test.ts @@ -1015,6 +1015,25 @@ layer("BitbucketPullRequestApi.layer", (it) => { }), ); + it.effect("still fails the permission read on a failure that is not the removed endpoint", () => + Effect.gen(function* () { + mockedRequest.mockReturnValue( + Effect.fail( + new BitbucketApi.BitbucketResponseError({ + operation: "request", + status: 401, + responseBodyLength: 0, + }), + ), + ); + const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; + + const error = yield* Effect.flip(api.getRepositoryPermission({ repository: "acme/web" })); + + assert.strictEqual(error._tag, "BitbucketResponseError"); + }), + ); + it.effect( "reads a removed permissions endpoint as granted rather than failing the merge on it", () => @@ -1033,28 +1052,12 @@ layer("BitbucketPullRequestApi.layer", (it) => { const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; assert.isTrue(yield* api.getRepositoryPermission({ repository: "acme/web" })); + // Gone for good, so the next check does not ask again. + assert.isTrue(yield* api.getRepositoryPermission({ repository: "acme/web" })); + expect(mockedRequest).toHaveBeenCalledTimes(1); }), ); - it.effect("still fails the permission read on a failure that is not the removed endpoint", () => - Effect.gen(function* () { - mockedRequest.mockReturnValue( - Effect.fail( - new BitbucketApi.BitbucketResponseError({ - operation: "request", - status: 401, - responseBodyLength: 0, - }), - ), - ); - const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; - - const error = yield* Effect.flip(api.getRepositoryPermission({ repository: "acme/web" })); - - assert.strictEqual(error._tag, "BitbucketResponseError"); - }), - ); - it.effect("reads the workspace's people and marks whoever is already a reviewer", () => Effect.gen(function* () { mockedRequest diff --git a/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.ts b/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.ts index 6dd04a4a0773..46bd02a73df4 100644 --- a/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.ts +++ b/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.ts @@ -606,6 +606,9 @@ export const make = Effect.gen(function* () { }, ); + /** Set by `getRepositoryPermission` once Bitbucket answers that its endpoint is gone. */ + let repositoryPermissionRemoved = false; + return BitbucketPullRequestApi.of({ getViewer: () => bitbucket.request({ method: "GET", url: "/user" }).pipe( @@ -665,17 +668,29 @@ export const make = Effect.gen(function* () { // permission being refused, so it is read the same way an unreachable read already is // elsewhere — as a permission that could not be learned, which grants rather than blocks, and // leaves the actual merge or write to say why if the account may not do it. Any other failure - // (a bad token, a network fault, an unreadable body) still fails as it did before. + // (a bad token, a network fault, an unreadable body) still fails as it did before. Once the + // configured Bitbucket has answered 410 it is not asked again while this server runs. getRepositoryPermission: (input) => withRepository(input.repository, () => - readPage({ - operation: "getRepositoryPermission", - url: `/user/permissions/repositories?q=${encodeURIComponent( - `repository.full_name="${filterLiteral(input.repository.trim())}"`, - )}`, - decode: decodeRepositoryPermissionJson, - }), - ).pipe(Effect.catchIf(isRepositoryPermissionRemovedError, () => Effect.succeed(true))), + Effect.suspend(() => + repositoryPermissionRemoved + ? Effect.succeed(true) + : readPage({ + operation: "getRepositoryPermission", + url: `/user/permissions/repositories?q=${encodeURIComponent( + `repository.full_name="${filterLiteral(input.repository.trim())}"`, + )}`, + decode: decodeRepositoryPermissionJson, + }), + ), + ).pipe( + Effect.catchIf(isRepositoryPermissionRemovedError, () => + Effect.sync(() => { + repositoryPermissionRemoved = true; + return true; + }), + ), + ), getPullRequestDiff: pullRequestDiff, diff --git a/packages/source-control-bitbucket/src/server/BitbucketPullRequestProvider.ts b/packages/source-control-bitbucket/src/server/BitbucketPullRequestProvider.ts index 3d26d5401602..1fa565902672 100644 --- a/packages/source-control-bitbucket/src/server/BitbucketPullRequestProvider.ts +++ b/packages/source-control-bitbucket/src/server/BitbucketPullRequestProvider.ts @@ -9,6 +9,7 @@ import { type ProviderChangeRequest, type ProviderChangeRequestActivity, type ProviderChangeRequestDetail, + type ProviderChangeRequestSummary, type PullRequestProviderApi, } from "@t3tools/source-control-core/server/PullRequestProvider"; import type { BitbucketPullRequest } from "./bitbucketPullRequestJson.ts"; @@ -233,6 +234,54 @@ export const make = Effect.gen(function* () { ); }, + // Everything a hover card shows is on the pull request itself. + getChangeRequestPreview: (input) => + api.getPullRequest({ repository: input.repository, number: input.number }).pipe( + Effect.mapError(fail("getChangeRequestPreview")), + Effect.map((pullRequest) => ({ + number: pullRequest.number, + title: pullRequest.title, + url: pullRequest.url, + author: pullRequest.author, + state: pullRequest.state, + isDraft: pullRequest.isDraft, + createdAt: pullRequest.createdAt, + })), + ), + + // The detail's line counts and conflict state, which a linked thread shows, without its + // checks and permission reads. + getChangeRequestSummary: (input) => { + const target = { repository: input.repository, number: input.number }; + return Effect.all( + [ + api.getPullRequest(target), + api.getDiffStat(target), + recoverRead(api.getMergeability(target), "unknown" as const), + ], + { concurrency: 3 }, + ).pipe( + Effect.mapError(fail("getChangeRequestSummary")), + Effect.map(([pullRequest, diffStat, mergeability]): ProviderChangeRequestSummary => ({ + number: pullRequest.number, + title: pullRequest.title, + url: pullRequest.url, + author: pullRequest.author, + headBranch: pullRequest.headBranch, + baseBranch: pullRequest.baseBranch, + state: pullRequest.state, + isDraft: pullRequest.isDraft, + mergeability, + additions: diffStat.additions, + deletions: diffStat.deletions, + changedFiles: diffStat.changedFiles, + mergedAt: null, + closedAt: null, + updatedAt: pullRequest.updatedAt, + })), + ); + }, + getChangeRequestActivity: (input) => { const target = { repository: input.repository, number: input.number }; return Effect.all( diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts index 0603dfec80ec..cd01e519196f 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts @@ -692,6 +692,162 @@ describe("GitCafePullRequestProvider", () => { }).pipe(Effect.provide(server.layer)); }); + it.effect.each([ + "changes", + "diff-files", + "partial-changes", + "partial-diff-files", + "omitted-diff-files", + "oversized-file", + ] as const)( + "retries line stats after a failed or incomplete %s read and retains the complete retry", + (failedRead) => { + const snapshot = { version: 4, headOid, comparisonBaseOid: baseOid }; + let recovered = false; + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": pull, + "GET /repos/owner/repo/pulls/7/status": { + merge: { conflicts: "unknown", fastForward: true, strategies: ["merge"] }, + checks: { pending: 0, failing: 0, total: 0 }, + }, + [`GET /repos/owner/repo/commits/${headOid}/checks`]: { items: [], next: null }, + "GET /repos/owner/repo/pulls/7/changes": () => + !recovered && failedRead === "changes" + ? {} + : { + ...snapshot, + items: [ + failedRead === "partial-changes" + ? { path: "file.ts", additions: 5, ...(recovered ? { deletions: 2 } : {}) } + : { path: "file.ts" }, + ...(failedRead === "partial-changes" + ? [{ path: "image.png", binary: true }] + : []), + ...(failedRead === "oversized-file" + ? [{ path: "large.ts", ...(recovered ? { binary: true } : { tooLarge: true }) }] + : []), + ], + next: null, + }, + "POST /repos/owner/repo/pulls/7/diff-files": () => + !recovered && failedRead === "diff-files" + ? {} + : { + ...snapshot, + items: + !recovered && failedRead === "omitted-diff-files" + ? [] + : [ + { + path: "file.ts", + additions: 5, + ...(!recovered && failedRead === "partial-diff-files" + ? {} + : { deletions: 2 }), + }, + ], + }, + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + const firstReadStart = server.sent.length; + assert.deepInclude(yield* provider.getChangeRequest(target), { + additions: failedRead.startsWith("partial-") || failedRead === "oversized-file" ? 5 : 0, + deletions: failedRead === "oversized-file" ? 2 : 0, + changedFiles: + failedRead === "changes" + ? 0 + : failedRead === "partial-changes" || failedRead === "oversized-file" + ? 2 + : 1, + }); + assert.strictEqual( + server.sent.length - firstReadStart, + failedRead === "changes" || failedRead === "partial-changes" ? 4 : 5, + ); + recovered = true; + for (let read = 0; read < 2; read++) { + const readStart = server.sent.length; + assert.deepInclude(yield* provider.getChangeRequest(target), { + additions: 5, + deletions: 2, + changedFiles: + failedRead === "partial-changes" || failedRead === "oversized-file" ? 2 : 1, + }); + assert.strictEqual( + server.sent.length - readStart, + read === 1 ? 3 : failedRead === "partial-changes" ? 4 : 5, + ); + } + assert.strictEqual( + server.sent.filter((request) => request.path.endsWith("/changes")).length, + 2, + ); + assert.strictEqual( + server.sent.filter((request) => request.path.endsWith("/diff-files")).length, + failedRead === "partial-changes" ? 0 : failedRead === "changes" ? 1 : 2, + ); + }).pipe(Effect.provide(server.layer)); + }, + ); + + it.effect("retries line stats when a changes page leaves an unread cursor", () => { + const snapshot = { version: 4, headOid, comparisonBaseOid: baseOid }; + let reads = 0; + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": pull, + "GET /repos/owner/repo/pulls/7/status": { + merge: { conflicts: "unknown", fastForward: true, strategies: ["merge"] }, + checks: { pending: 0, failing: 0, total: 0 }, + }, + [`GET /repos/owner/repo/commits/${headOid}/checks`]: { items: [], next: null }, + "GET /repos/owner/repo/pulls/7/changes": () => + reads++ === 0 + ? { ...snapshot, items: [], next: "unread" } + : { ...snapshot, items: [{ path: "file.ts", additions: 5, deletions: 2 }], next: null }, + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + assert.deepInclude(yield* provider.getChangeRequest(target), { + additions: 0, + changedFiles: 0, + }); + assert.deepInclude(yield* provider.getChangeRequest(target), { + additions: 5, + changedFiles: 1, + }); + assert.strictEqual(reads, 2); + }).pipe(Effect.provide(server.layer)); + }); + + it.effect.each([baseOid, null])( + "does not retain line stats for a comparison base different from %s", + (observedBaseOid) => { + let reads = 0; + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": { ...pull, observedBaseOid }, + "GET /repos/owner/repo/pulls/7/status": { + merge: { conflicts: "unknown", fastForward: true, strategies: ["merge"] }, + checks: { pending: 0, failing: 0, total: 0 }, + }, + [`GET /repos/owner/repo/commits/${headOid}/checks`]: { items: [], next: null }, + "GET /repos/owner/repo/pulls/7/changes": () => ({ + version: 4, + headOid, + comparisonBaseOid: "another-base", + items: [{ path: "file.ts", additions: ++reads, deletions: 0 }], + next: null, + }), + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + assert.strictEqual((yield* provider.getChangeRequest(target)).additions, 1); + assert.strictEqual((yield* provider.getChangeRequest(target)).additions, 2); + assert.strictEqual(reads, 2); + }).pipe(Effect.provide(server.layer)); + }, + ); + it.effect("fences a native merge on the target branch's current commit", () => { const branchOid = "fedcba9876543210fedcba9876543210fedcba98"; const server = fakeGitCafe({ diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts index 4bbd54b73cf9..de331b519afd 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts @@ -1,5 +1,8 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; +import * as Cache from "effect/Cache"; +import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; import * as Option from "effect/Option"; import * as Schema from "effect/Schema"; import { randomUuidV4 } from "@t3tools/provider-core/server/randomUuid"; @@ -55,6 +58,8 @@ const DIFF_FILES_BATCH = 64; const LINE_STATS_MAX_FILES = DIFF_FILES_BATCH * 4; /** Structural `/changes` pages, 500 files each, read to count a pull request's files. */ const MAX_CHANGE_PAGES = 20; +/** A viewer's actor id is stable, so involvement filters skip `/filter-options` for this long. */ +const ACTOR_TTL = Duration.minutes(10); const chunk = (items: ReadonlyArray, size: number): Array> => Array.from({ length: Math.ceil(items.length / size) }, (_, index) => @@ -136,7 +141,8 @@ const StackOutcome = Schema.Struct({ id: Schema.String, state: Schema.String }); /** Where a stack operation stopped short; anything else is underway or done. */ const FAILED_STACK_STATES = new Set(["failed", "stopped", "cancelled", "reconciliation_required"]); -type PullRef = ProviderRepositoryRef & { readonly number: number }; +type RepositoryRef = Pick; +type PullRef = RepositoryRef & { readonly number: number }; type Paged = { readonly items: ReadonlyArray; readonly next: string | null }; /** The heads a reader saw are still every unmerged layer the action moves. */ @@ -188,7 +194,7 @@ export const make = Effect.gen(function* () { /** The repository's API root, refused before anything is sent for a host GitCafe is not on. */ const repositoryPath = ( operation: string, - input: ProviderRepositoryRef, + input: RepositoryRef, ): Effect.Effect => GitCafeHosts.isGitCafeHost(input.host) && GitCafeHosts.gitCafeRepositoryPath(input.repository) === input.repository @@ -385,6 +391,7 @@ export const make = Effect.gen(function* () { const readLineStats = Effect.fn("GitCafePullRequestProvider.readLineStats")(function* ( input: PullRef, version: number, + headOid: string, ) { const path = yield* pullPath("getChangeRequest", input); const files: Array = []; @@ -399,6 +406,12 @@ export const make = Effect.gen(function* () { { path: `${path}/changes?${query}` }, Changes, ); + if ( + batch.version !== version || + batch.headOid !== headOid || + (revision !== undefined && batch.comparisonBaseOid !== revision.baseOid) + ) + return yield* failure("getChangeRequest", "GitCafe answered from a different revision."); revision ??= { version: batch.version, headOid: batch.headOid, @@ -411,23 +424,61 @@ export const make = Effect.gen(function* () { } // A hunk read that loses a race with a push leaves the counts at zero rather than dropping // the file count with them. - const counted = + const details: Option.Option>> = files.some((file) => file.additions !== undefined) || files.length > LINE_STATS_MAX_FILES || revision === undefined - ? files - : (yield* Effect.forEach( + ? Option.some([files]) + : yield* Effect.forEach( chunk(detailablePaths(files), DIFF_FILES_BATCH), (paths) => readDiffFiles(input, revision, paths), { concurrency: 4 }, - ).pipe(Effect.orElseSucceed(() => [[]]))).flat(); + ).pipe(Effect.option); + const counted: ReadonlyArray = Option.isSome(details) + ? details.value.flat() + : []; return { + revision, + complete: + Option.isSome(details) && + after === null && + withDetails(files, counted).every( + (file) => file.binary || (file.additions !== undefined && file.deletions !== undefined), + ), changedFiles: files.length, additions: counted.reduce((total, file) => total + (file.additions ?? 0), 0), deletions: counted.reduce((total, file) => total + (file.deletions ?? 0), 0), }; }); + /** + * Line totals held by the revision they count, which never changes, so reopening an unchanged + * pull request skips the `/changes` pages and hunk reads. Only a complete count of the head + * and base the pull reported is held. + */ + const lineStats = yield* Cache.makeWith( + (key: string) => { + const [host = "", repository = "", number = "", version = "", headOid = "", baseOid = ""] = + key.split("\0"); + return readLineStats( + { host, repository, number: Number(number) }, + Number(version), + headOid, + ).pipe( + Effect.map(({ revision, complete, ...stats }) => ({ + stats, + // The pages already agree with the version and head asked for; the base is the pull's. + held: complete && revision?.baseOid === baseOid, + })), + ); + }, + { + capacity: 64, + timeToLive: (exit) => + Exit.isSuccess(exit) && exit.value.held ? Duration.infinity : Duration.zero, + }, + ); + const merge = Effect.fn("GitCafePullRequestProvider.merge")(function* ( input: Parameters[0], ) { @@ -476,6 +527,29 @@ export const make = Effect.gen(function* () { return yield* failure("merge", outcome.reason ?? "GitCafe could not merge the pull request."); }); + /** The viewer's actor on a repository; a viewer GitCafe knows no actor for is asked again. */ + const viewerActors = yield* Cache.makeWith( + (key: string) => { + const [host = "", repository = "", viewer = ""] = key.split("\0"); + return read( + "listChangeRequests", + { host }, + { path: `/repos/${repository}/filter-options` }, + FilterOptions, + ).pipe( + Effect.map( + (options) => + options.actors.find((actor) => actor.handle.toLowerCase() === viewer)?.actorId, + ), + ); + }, + { + capacity: 64, + timeToLive: (exit) => + Exit.isSuccess(exit) && exit.value !== undefined ? ACTOR_TTL : Duration.zero, + }, + ); + const provider: PullRequestProviderApi = { kind: SourceControlProviderKind.make("gitcafe"), capabilities: CAPABILITIES, @@ -495,11 +569,9 @@ export const make = Effect.gen(function* () { // A host narrows as far as it can: when the actor list can't be read, the listing comes // back unnarrowed and the page's own involvement filter does the rest. A refused token // or a rate limit still fails, since the service acts on those for the whole host. - const options = yield* read( - "listChangeRequests", - input, - { path: `${base}/filter-options` }, - FilterOptions, + const actor = yield* Cache.get( + viewerActors, + `${input.host}\0${input.repository}\0${input.viewer.toLowerCase()}`, ).pipe( Effect.map(Option.some), Effect.catchTags({ @@ -509,10 +581,8 @@ export const make = Effect.gen(function* () { : Effect.succeed(Option.none()), }), ); - if (Option.isSome(options)) { - actorId = options.value.actors.find( - (actor) => actor.handle.toLowerCase() === input.viewer.toLowerCase(), - )?.actorId; + if (Option.isSome(actor)) { + actorId = actor.value; // A viewer GitCafe knows no actor for has authored and been asked to review nothing. if (actorId === undefined) return { items: [], truncated: false, continues: false }; } @@ -584,7 +654,20 @@ export const make = Effect.gen(function* () { ), pull.headOid === null ? Effect.succeed(null) - : readLineStats(input, pull.version).pipe(Effect.orElseSucceed(() => null)), + : Cache.get( + lineStats, + [ + input.host, + input.repository, + input.number, + pull.version, + pull.headOid, + pull.observedBaseOid ?? "", + ].join("\0"), + ).pipe( + Effect.map(({ stats }) => stats), + Effect.orElseSucceed(() => null), + ), ], { concurrency: 3 }, ); @@ -634,6 +717,11 @@ export const make = Effect.gen(function* () { : permissions, }; }), + // A hover card needs only the pull, not detail's status, checks and line counts. + getChangeRequestPreview: (input) => + readPull("getChangeRequestPreview", input).pipe( + Effect.map((pull) => Json.toChangeRequest(pull, input)), + ), getChangeRequestStack: (input) => readStack("getChangeRequestStack", input).pipe( Effect.map((stack) => (stack === null ? null : Json.toStack(stack, input))), diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 961106c58acc..4403c997f9ea 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -5,7 +5,9 @@ import * as Effect from "effect/Effect"; import * as Clock from "effect/Clock"; import * as Deferred from "effect/Deferred"; import * as Fiber from "effect/Fiber"; +import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; +import * as ChildProcessSpawner from "effect/process/ChildProcessSpawner"; import * as Schema from "effect/Schema"; import * as TestClock from "effect/testing/TestClock"; import * as Redacted from "effect/Redacted"; @@ -184,12 +186,13 @@ function listedPullRequests( count: number, firstNumber: number, overrides?: (number: number) => Readonly>, + endCursor: string | null = null, ): string { return encodeJson({ data: { repository: { pullRequests: { - pageInfo: { hasNextPage: false }, + pageInfo: { hasNextPage: endCursor !== null, endCursor }, nodes: rows(count, firstNumber, overrides), }, }, @@ -551,6 +554,241 @@ it.effect( }), ); +it.effect("separates overlapping unpinned permission reads after a saved account switch", () => + Effect.gen(function* () { + const started = yield* Deferred.make(); + const broadStarted = yield* Deferred.make(); + const release = yield* Deferred.make(); + let activeToken = "restricted-credential"; + const requests: Array<{ readonly url: string; readonly authorization: string | undefined }> = + []; + const credentials = Layer.succeed( + GitHubCredentials.GitHubCredentials, + GitHubCredentials.GitHubCredentials.of({ + get: (host) => + Effect.sync(() => ({ + host, + token: Redacted.make(activeToken), + source: "settings" as const, + fingerprint: `${host}:${activeToken.length}${activeToken.at(-1)}`, + })), + invalidate: () => Effect.void, + }), + ); + const http = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make((request) => + Effect.gen(function* () { + const authorization = request.headers["authorization"]; + requests.push({ url: request.url, authorization }); + if (authorization === "Bearer broad-credential") { + yield* Deferred.succeed(broadStarted, undefined); + } + yield* Deferred.succeed(started, undefined); + yield* Deferred.await(release); + return HttpClientResponse.fromWeb( + request, + new Response( + encodeJson( + coreResponse({ + viewerCanUpdateBranch: authorization === "Bearer broad-credential", + }), + ), + ), + ); + }), + ), + ); + const cli = yield* GitHubPullRequestApi.make.pipe( + Effect.provide( + GitHubApi.layer.pipe( + Layer.provide(Layer.mergeAll(credentials, http)), + Layer.provide(GitHubQuota.layer), + Layer.provide(SourceControlRateLimit.layer), + Layer.merge(TestSourceControlHost.layer()), + Layer.provideMerge(NodeServices.layer), + ), + ), + ); + const input = { cwd: "/repo", host: "github.com", repository: "acme/web", number: 7 }; + // The single-environment permission path is interactive but does not pin an account. + const read = cli + .getPullRequestDetail(input) + .pipe(Effect.provideService(SourceControlRateLimit.Interactive, true)); + const older = yield* read.pipe(Effect.forkChild); + yield* Deferred.await(started); + activeToken = "broad-credential"; + const fresh = yield* Effect.all([read, read], { concurrency: "unbounded" }).pipe( + Effect.forkChild, + ); + // Keep both accounts' HTTP requests pending before answering either one. + yield* Deferred.await(broadStarted); + yield* Deferred.succeed(release, undefined); + + expect((yield* Fiber.join(older)).comparison?.viewerCanUpdate).toBe(false); + expect((yield* Fiber.join(fresh)).map((detail) => detail.comparison?.viewerCanUpdate)).toEqual([ + true, + true, + ]); + // Two accounts need two reads; the new account's panel readers still share one. + expect(requests.map((request) => request.authorization)).toEqual([ + "Bearer restricted-credential", + "Bearer broad-credential", + ]); + expect(requests.every((request) => request.url.endsWith("/graphql"))).toBe(true); + }), +); + +it.effect.each([false, true])( + "starts fresh reads after a stack rebase (partial failure: %s)", + (partialFailure) => + Effect.gen(function* () { + const detailStarted = yield* Deferred.make(); + const runsStarted = yield* Deferred.make(); + const freshRunsStarted = yield* Deferred.make(); + const release = yield* Deferred.make(); + let pushed = false; + let rebases = 0; + let pushes = 0; + const input = { cwd: "/w", repository: "acme/web", host: "github.com", number: 7 }; + mockedExecute.mockImplementation((call) => + Effect.gen(function* () { + if (call.kind === "rest" && call.path.includes("/stacks?")) { + return output( + encodeJson([ + { + number: 50, + url: "https://api.github.com/repos/acme/web/stacks/50", + base: { ref: "main" }, + pull_requests: [ + { number: 7, head: { ref: "feature", sha: "abc123" }, state: "open" }, + { number: 8, head: { ref: "top", sha: "def456" }, state: "open" }, + ], + }, + ]), + ); + } + if (call.operation === "runGitHubStackAction") { + return output( + encodeJson({ + data: { + repository: { + pr7: { + headRepository: { viewerPermission: "WRITE" }, + maintainerCanModify: false, + }, + pr8: { + headRepository: { viewerPermission: "WRITE" }, + maintainerCanModify: false, + }, + }, + }, + }), + ); + } + if (call.kind === "graphql" && call.query.includes("headRefName: $head")) { + if (pushed) yield* Deferred.await(freshRunsStarted); + return output(encodeJson(heads(pushed ? [] : [7]))); + } + const beforePush = !pushed; + if (beforePush) { + yield* Deferred.succeed(call.kind === "rest" ? runsStarted : detailStarted, undefined); + yield* Deferred.await(release); + } + if (call.kind === "rest" && !beforePush) { + yield* Deferred.succeed(freshRunsStarted, undefined); + } + return output( + encodeJson( + call.kind === "rest" + ? workflowRuns(beforePush ? [10] : []) + : coreResponse({ headRefOid: beforePush ? "abc123" : "new-sha" }), + ), + ); + }), + ); + const cli = yield* GitHubPullRequestApi.make.pipe( + Effect.provide( + Layer.mergeAll( + mockApi.pipe(Layer.provide(GitHubQuota.layer), Layer.provide(NodeCrypto.layer)), + FileSystem.layerNoop({ + makeTempDirectoryScoped: () => Effect.succeed("/scratch"), + }), + TestSourceControlHost.layer({ + process: { + run: (request) => + Effect.sync(() => { + expect(request.command).toBe("git"); + expect(request.operation).toBe("cascadeRebaseStack"); + if (request.args[0] === "push") { + pushed = true; + pushes++; + } + const conflict = + request.args[0] === "rebase" && + request.args[1] !== "--abort" && + ++rebases === 2 && + partialFailure; + return TestSourceControlHost.processOutput( + ["rev-parse", "merge-base"].includes(request.args[0]!) ? "new-sha\n" : "", + { exitCode: ChildProcessSpawner.ExitCode(conflict ? 1 : 0) }, + ); + }), + }, + }), + ), + ), + ); + const workflowInput = { + ...input, + headSha: "abc123", + headBranch: "feat/page", + headRepositoryOwner: "octocat", + isCrossRepository: true as const, + }; + const olderDetail = yield* cli.getPullRequestDetail(input).pipe(Effect.forkChild); + const olderRuns = yield* cli + .listWorkflowRunsRequiringApproval(workflowInput) + .pipe(Effect.forkChild); + yield* Deferred.await(detailStarted); + yield* Deferred.await(runsStarted); + const action = yield* cli + .runPullRequestAction({ + ...input, + number: 8, + stackNumber: 50, + action: "update-branch", + expectedStackHeads: [ + { number: 7, headSha: "abc123" }, + { number: 8, headSha: "def456" }, + ], + }) + .pipe(Effect.result); + expect(action._tag).toBe(partialFailure ? "Failure" : "Success"); + if (partialFailure) { + expect(action).toMatchObject({ + failure: { _tag: "GitHubStackRebaseFailedError", completed: 1 }, + }); + } + expect(pushes).toBe(partialFailure ? 1 : 2); + expect((yield* cli.getPullRequestDetail(input)).headSha).toBe("new-sha"); + expect( + yield* cli.listWorkflowRunsRequiringApproval(workflowInput).pipe(Effect.result), + ).toMatchObject({ + _tag: "Failure", + failure: { _tag: "GitHubWorkflowApprovalRefusedError", reason: "head-not-unique" }, + }); + // The stack action still needs only its membership and permission reads. + expect( + mockedExecute.mock.calls.filter(([call]) => call.operation === "runGitHubStackAction"), + ).toHaveLength(2); + expect(mockedExecute.mock.calls).toHaveLength(8); + yield* Deferred.succeed(release, undefined); + expect((yield* Fiber.join(olderDetail)).headSha).toBe("abc123"); + expect((yield* Fiber.join(olderRuns)).map((run) => run.id)).toEqual([10]); + }), +); + layer("GitHubPullRequestApi.layer", (it) => { it.effect("holds background previews once an answer reports the reserve, until it resets", () => Effect.gen(function* () { @@ -1408,10 +1646,10 @@ layer("GitHubPullRequestApi.layer", (it) => { Effect.gen(function* () { mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); mockedExecute.mockReturnValueOnce( - Effect.succeed(output(listedPullRequests(3, 1, () => ({ isDraft: true })))), + Effect.succeed(output(listedPullRequests(3, 1, () => ({ isDraft: true }), "c3"))), ); mockedExecute.mockReturnValueOnce( - Effect.succeed(output(listedPullRequests(6, 1, (number) => ({ isDraft: number < 4 })))), + Effect.succeed(output(listedPullRequests(3, 4, () => ({ isDraft: false }), "c6"))), ); mockedStackMemberships.mockReturnValueOnce( Effect.succeed( @@ -1446,6 +1684,7 @@ layer("GitHubPullRequestApi.layer", (it) => { expect(batch.items[1]?.stack).toBeUndefined(); expect(batch.truncated).toBe(true); expect(batch.continues).toBe(false); + expect(mockedExecute).toHaveBeenCalledTimes(3); expect(mockedStackMemberships).toHaveBeenCalledTimes(1); const membership = mockedStackMemberships.mock.calls[0]?.[0]; const numbers = Object.entries( @@ -2048,18 +2287,26 @@ layer("GitHubPullRequestApi.layer", (it) => { Effect.gen(function* () { const unrelated = () => ({ reviewRequests: [{ login: "somebody-else" }] }); mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); - mockedExecute.mockReturnValueOnce( - Effect.succeed(output(listedPullRequests(3, 1, unrelated))), - ); mockedExecute.mockReturnValueOnce( Effect.succeed( output( - listedPullRequests(4, 1, (number) => - number === 4 ? { reviewRequests: [{ login: "bilal" }] } : unrelated(), + listedPullRequests( + 3, + 1, + (number) => ({ + ...unrelated(), + ...(number === 1 ? { number: null } : {}), + }), + "c3", ), ), ), ); + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output(listedPullRequests(1, 4, () => ({ reviewRequests: [{ login: "bilal" }] }))), + ), + ); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; const batch = yield* cli.listPullRequests({ @@ -2073,12 +2320,70 @@ layer("GitHubPullRequestApi.layer", (it) => { }); expect(batch.items.map((item) => item.number)).toEqual([4]); + // The malformed row still counts toward the requested range. expect(queryAt(1)).toContain("first: 3,"); - expect(queryAt(2)).toContain("first: 6,"); + expect(queryAt(2)).toContain("first: 3,"); + expect(varsAt(1)["after"]).toBeNull(); + expect(varsAt(2)["after"]).toBe("c3"); + expect(mockedExecute).toHaveBeenCalledTimes(3); assert.isFalse(batch.truncated); }), ); + it.effect("does not restart an exhausted search-free fallback at a full filtered boundary", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output(listedPullRequests(3, 1, () => ({ isDraft: true })))), + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + state: "open", + involvement: "all", + viewer: "bilal", + limit: 2, + filters: { draft: "hide" }, + }); + + expect(mockedExecute).toHaveBeenCalledTimes(2); + expect(batch.items).toEqual([]); + assert.isFalse(batch.truncated); + assert.isFalse(batch.continues); + }), + ); + + it.effect("keeps the repeated-cursor guard while widening a search-free fallback", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output(listedPullRequests(3, 1, () => ({ isDraft: true }), "same"))), + ); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output(listedPullRequests(3, 4, () => ({ isDraft: true }), "same"))), + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + state: "open", + involvement: "all", + viewer: "bilal", + limit: 2, + filters: { draft: "hide" }, + }); + + expect(mockedExecute).toHaveBeenCalledTimes(3); + expect(varsAt(2)["after"]).toBe("same"); + expect(batch.items).toEqual([]); + assert.isTrue(batch.truncated); + assert.isFalse(batch.continues); + }), + ); + it.effect("bounds a sparse search-free fallback and reports the unread tail", () => Effect.gen(function* () { // A repository with far more pull requests than the bound, none of them for this reader. @@ -2116,10 +2421,17 @@ layer("GitHubPullRequestApi.layer", (it) => { limit: 2, }); - // The last widening reads a thousand rows, a page of a hundred at a time, and stops there. + // Widening continues from the preceding cursor and reads no row a second time. const last = mockedExecute.mock.calls.length - 1; - expect(varsAt(last)["after"]).toBe(`c${listed - 100}`); - expect(listed - (3 + 6 + 12 + 24 + 48 + 96 + 192 + 384 + 768)).toBe(1000); + expect(varsAt(last)["after"]).toBe("c968"); + expect(queryAt(last)).toContain("first: 32,"); + expect(listed).toBe(1000); + expect(mockedExecute).toHaveBeenCalledTimes(17); + expect( + mockedExecute.mock.calls + .slice(2) + .every(([call]) => call.kind === "graphql" && call.variables?.["after"] !== null), + ).toBe(true); assert.strictEqual(batch.items.length, 0); assert.isTrue(batch.truncated); }), @@ -3617,6 +3929,78 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); + it.effect("starts a fresh detail read after a mutation while an older read is pending", () => + Effect.gen(function* () { + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + let detailReads = 0; + mockedExecute.mockImplementation((call) => + Effect.gen(function* () { + expect(call.host).toBe("github.detail-after-write.test"); + if (call.kind === "graphql" && call.query.trimStart().startsWith("mutation")) { + return output("{}"); + } + if (call.kind === "graphql" && call.query.includes(NODE_ID_QUERY)) { + return output(encodeJson(nodeIdAnswer("PR_after_write"))); + } + const title = ++detailReads === 1 ? "Before mutation" : "After mutation"; + if (detailReads === 1) { + yield* Deferred.succeed(started, undefined); + yield* Deferred.await(release); + } + return output(encodeJson(coreResponse({ title }))); + }), + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const input = { + cwd: "/w\n%repo", + repository: "acme/web", + host: "github.detail-after-write.test", + number: 7, + }; + + const older = yield* cli.getPullRequestDetail(input).pipe(Effect.forkChild); + yield* Deferred.await(started); + yield* cli.updatePullRequest({ ...input, title: "After mutation" }); + const fresh = yield* cli.getPullRequestDetail(input); + + expect(fresh.title).toBe("After mutation"); + expect(detailReads).toBe(2); + expect(variablesOf("updatePullRequest(")).toEqual([ + { pullRequestId: "PR_after_write", title: "After mutation" }, + ]); + yield* Deferred.succeed(release, undefined); + expect((yield* Fiber.join(older)).title).toBe("Before mutation"); + }), + ); + + it.effect("uses the node id from core detail for the first mutation without another lookup", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce( + Effect.succeed(output(encodeJson(coreResponse({ id: "PR_from_core" })))), + ) + .mockReturnValue(Effect.succeed(output("{}"))); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const input = { + cwd: "/w", + repository: "acme/web", + host: "github.core-node-id.test", + number: 7, + }; + + const detail = yield* cli.getPullRequestDetail(input); + yield* cli.updatePullRequest({ ...input, title: "Uses core id" }); + + expect(detail).not.toHaveProperty("nodeId"); + expect(mockedExecute).toHaveBeenCalledTimes(2); + expect(variablesOf(NODE_ID_QUERY)).toEqual([]); + expect(variablesOf("updatePullRequest(")).toEqual([ + { pullRequestId: "PR_from_core", title: "Uses core id" }, + ]); + }), + ); + it.effect("keeps the core detail read separate from conversation activity", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce( diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index b8c7fe227688..b6447ca3b35e 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -9,6 +9,7 @@ import * as Effect from "effect/Effect"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; import * as Exit from "effect/Exit"; +import * as Fiber from "effect/Fiber"; import * as Option from "effect/Option"; import * as Request from "effect/Request"; import * as RequestResolver from "effect/RequestResolver"; @@ -1104,7 +1105,67 @@ const SIMPLE_ACTION_MUTATIONS = { /** @public Service construction is part of the canonical Effect module API. */ export const make = Effect.gen(function* () { - const api = yield* GitHubApi.GitHubApi; + const transport = yield* GitHubApi.GitHubApi; + // Opening a pull request asks GitHub the same questions from its detail, its checks and the + // viewer's permissions at once. A read that starts while an identical one is in flight under the + // same credential takes its answer; nothing is kept once it lands. A write, or a stack push, + // drops what is in flight, so a read started after it asks GitHub again. + const restReads = new Map< + string, + Fiber.Fiber + >(); + const graphqlReads = new Map>(); + const forgetReads = Effect.sync(() => { + restReads.clear(); + graphqlReads.clear(); + }); + const encodeReadKey = Schema.encodeEffect(Schema.fromJsonString(Schema.Unknown)); + const shareRead = ( + reads: Map>, + send: (input: I) => Effect.Effect, + input: I, + ) => + Effect.gen(function* () { + const host = input.host.trim().toLowerCase(); + const { token, fingerprint } = yield* transport.credential(host); + // Every read input is plain data, so it always encodes. + const key = yield* encodeReadKey([ + fingerprint, + yield* SourceControlRateLimit.CredentialScope, + yield* SourceControlRateLimit.Interactive, + input, + ]).pipe(Effect.orDie); + const held = reads.get(key); + if (held !== undefined) return yield* Fiber.join(held); + // Pinned, so whoever joins gets the answer of the credential the key names. + const fiber = yield* send(input).pipe( + Effect.provideService(GitHubApi.PinnedGitHubCredential, { + host, + token, + credentialFingerprint: fingerprint, + }), + Effect.forkDetach, + ); + reads.set(key, fiber); + return yield* Fiber.join(fiber).pipe( + Effect.ensuring( + Effect.sync(() => { + if (reads.get(key) === fiber) reads.delete(key); + }), + ), + ); + }); + const api = GitHubApi.GitHubApi.of({ + ...transport, + graphql: (input) => + input.query.trimStart().startsWith("mutation") + ? transport.graphql(input).pipe(Effect.ensuring(forgetReads)) + : shareRead(graphqlReads, transport.graphql, input), + rest: (input) => + input.method === undefined || input.method === "GET" + ? shareRead(restReads, transport.rest, input) + : transport.rest(input).pipe(Effect.ensuring(forgetReads)), + }); const sourceControlHost = yield* SourceControlHost.SourceControlHost; const fileSystem = yield* FileSystem.FileSystem; const revalidateChecks = yield* makeChecksRevalidator; @@ -1202,6 +1263,15 @@ export const make = Effect.gen(function* () { */ const nodeIds = new Map(); + const rememberNodeId = (key: string, nodeId: string) => { + nodeIds.delete(key); + if (nodeIds.size >= NODE_ID_CACHE_CAPACITY) { + const oldest = nodeIds.keys().next().value; + if (oldest !== undefined) nodeIds.delete(oldest); + } + nodeIds.set(key, nodeId); + }; + const pullRequestNodeId = (input: { readonly cwd: string; readonly repository: string; @@ -1228,17 +1298,7 @@ export const make = Effect.gen(function* () { variables: { owner, name, number: input.number }, query: PULL_REQUEST_NODE_ID_GRAPHQL_QUERY, decode: decodePullRequestNodeIdJson, - }).pipe( - Effect.tap((nodeId) => - Effect.sync(() => { - if (nodeIds.size >= NODE_ID_CACHE_CAPACITY) { - const oldest = nodeIds.keys().next().value; - if (oldest !== undefined) nodeIds.delete(oldest); - } - nodeIds.set(key, nodeId); - }), - ), - ); + }).pipe(Effect.tap((nodeId) => Effect.sync(() => rememberNodeId(key, nodeId)))); }; /** @@ -1542,6 +1602,14 @@ export const make = Effect.gen(function* () { query: pullRequestCoreGraphQlQuery(input.host), decode: decodePullRequestCoreJson, }).pipe( + Effect.tap((core) => { + const nodeId = core.nodeId; + return nodeId === undefined + ? Effect.void + : Effect.sync(() => + rememberNodeId(`${input.host} ${owner}/${name} ${input.number}`, nodeId), + ); + }), // Past a hundred checks the first page would let the rest imply success, so the whole // rollup is walked instead. Effect.filterOrElse( @@ -1564,6 +1632,7 @@ export const make = Effect.gen(function* () { })), ), ), + Effect.map(({ nodeId: _nodeId, ...core }) => core), ), ), ); @@ -1897,82 +1966,73 @@ export const make = Effect.gen(function* () { }), ); } - /** Pages of up to a hundred until `rows` have arrived or GitHub has no more. */ - const collect = ( - rows: number, - page: ( - after: string | null, - rows: number, - ) => Effect.Effect, - ) => + const read = ( + continues: boolean, + ): Effect.Effect => Effect.gen(function* () { - const countRows = (pages: ReadonlyArray) => - pages.reduce((total, read) => total + read.rawCount, 0); - const { pages, truncated } = yield* readGraphQlPages( - (after, pages: ReadonlyArray) => - page(after, rows - countRows(pages)), + let requestedRows = input.limit + 1; + let rawCount = 0; + const items: GitHubPullRequestListItem[] = []; + const { truncated: cutShort } = yield* readGraphQlPages( + (after): Effect.Effect => { + const rows = requestedRows - rawCount; + const page = continues + ? graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "listPullRequests", + // The reader's own words are in the query. + variables: { q: query, after }, + query: pullRequestSearchGraphQlQuery(rows, false, true), + decode: decodePullRequestSearchJson, + }) + : graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "listPullRequests", + variables: { owner, name, states: LIST_STATES[input.state], after }, + query: pullRequestListGraphQlQuery(rows), + decode: decodePullRequestListJson, + }); + return page.pipe( + Effect.tap((read) => + Effect.sync(() => { + rawCount += read.rawCount; + items.push( + ...(continues + ? read.items + : read.items.filter((item) => matchesUnsortedListing(item, input))), + ); + if ( + !continues && + items.length < input.limit && + rawCount >= requestedRows && + requestedRows < fallbackMaxRows + ) { + // Widen the target while keeping the pages and cursors already read. + requestedRows = Math.min(requestedRows * 2, fallbackMaxRows); + } + }), + ), + ); + }, { nextCursor: (read) => read.endCursor, - until: (pages) => countRows(pages) >= rows, + until: () => rawCount >= requestedRows, }, ); - // `truncated` also covers a cursor GitHub repeated before `rows` arrived. return { - items: pages.flatMap((read) => read.items), - rawCount: countRows(pages), - truncated, + items: items.slice(0, input.limit), + // One row over the page size is the probe for a next page, and it is + // counted before decoding: a skipped malformed row must not end paging. + truncated: + cutShort || + (continues + ? rawCount > input.limit + : items.length > input.limit || rawCount >= requestedRows), + continues, }; }); - const read = ( - continues: boolean, - requestedRows = input.limit + 1, - ): Effect.Effect => - collect(requestedRows, (after, rows) => - continues - ? graphqlRead({ - cwd: input.cwd, - host: input.host, - operation: "listPullRequests", - // The reader's own words are in the query. - variables: { q: query, after }, - query: pullRequestSearchGraphQlQuery(rows, false, true), - decode: decodePullRequestSearchJson, - }) - : graphqlRead({ - cwd: input.cwd, - host: input.host, - operation: "listPullRequests", - variables: { owner, name, states: LIST_STATES[input.state], after }, - query: pullRequestListGraphQlQuery(rows), - decode: decodePullRequestListJson, - }), - ).pipe( - Effect.flatMap(({ items: rawItems, rawCount, truncated: cutShort }) => { - const items = continues - ? rawItems - : rawItems.filter((item) => matchesUnsortedListing(item, input)); - if ( - !continues && - items.length < input.limit && - rawCount >= requestedRows && - requestedRows < fallbackMaxRows - ) { - const nextRows = Math.min(requestedRows * 2, fallbackMaxRows); - if (nextRows > requestedRows) return read(false, nextRows); - } - return Effect.succeed({ - items: items.slice(0, input.limit), - // One row over the page size is the probe for a next page, and it is - // counted before decoding: a skipped malformed row must not end paging. - truncated: - cutShort || - (continues - ? rawCount > input.limit - : items.length > input.limit || rawCount >= requestedRows), - continues, - }); - }), - ); // GitHub does not index every repository for search, and one it will not search answers // with no rows rather than with an error — so an empty listing is read again from the // repository's own list. Those rows come back newest-created first, an order no `updated:` @@ -2496,6 +2556,7 @@ export const make = Effect.gen(function* () { runPullRequestAction: (input) => { if (input.stackNumber !== undefined) return runGitHubStackAction({ ...input, stackNumber: input.stackNumber }).pipe( + Effect.ensuring(forgetReads), Effect.provideService(GitHubApi.GitHubApi, api), Effect.provideService(SourceControlHost.SourceControlHost, sourceControlHost), Effect.provideService(FileSystem.FileSystem, fileSystem), diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.ts index e3768b04d7bc..68d0a5d43b6b 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.ts @@ -708,6 +708,7 @@ const RawCoreSchema = Schema.Struct({ repository: Schema.Struct({ ...RawRepositoryAccessSchema.fields, pullRequest: Schema.Struct({ + id: Schema.optional(Schema.String), ...RawDetailSchema.fields, ...RawViewerFieldsSchema.fields, viewerCanUpdateBranch: Schema.Boolean, @@ -826,7 +827,7 @@ export const pullRequestCoreGraphQlQuery = (host: string) => { repository(owner: $owner, name: $name) { mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed viewerPermission pullRequest(number: $number) { - number title url body state isDraft mergeable reviewDecision + id number title url body state isDraft mergeable reviewDecision additions deletions changedFiles createdAt updatedAt mergedAt closedAt headRefName baseRefName headRefOid isCrossRepository headRepositoryOwner { login } @@ -2302,6 +2303,7 @@ export function decodePullRequestWatchFingerprintsJson( } export interface GitHubPullRequestCore extends GitHubPullRequestDetail { + readonly nodeId?: string; readonly viewerAccess: GitHubViewerAccess & GitHubRepositoryAccess; readonly comparison: GitHubBaseComparison | null; readonly checksTruncated: boolean; @@ -2316,6 +2318,7 @@ export function decodePullRequestCoreJson( const pr = repository.pullRequest; const contexts = pr.commits.nodes[0]?.commit.statusCheckRollup?.contexts; return Result.succeed({ + ...(pr.id === undefined ? {} : { nodeId: pr.id }), ...toDetail({ ...pr, reviewRequests: pr.reviewRequests.nodes.flatMap(({ requestedReviewer }) => diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts index b4546162f30c..c41ab18c6aac 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts @@ -5,6 +5,7 @@ import { ChildProcessSpawner } from "effect/process"; import * as GitLabCli from "./GitLabCli.ts"; import * as GitLabPullRequestCli from "./GitLabPullRequestCli.ts"; +import * as GitLabPullRequestProvider from "./GitLabPullRequestProvider.ts"; const mockedExecute = vi.fn(); @@ -1533,4 +1534,34 @@ layer("GitLabPullRequestCli.layer", (it) => { assert.strictEqual(mockedExecute.mock.calls.length, 3); }), ); + + it.effect( + "reads a project's merge settings once while it holds them, and never for a summary", + () => + Effect.gen(function* () { + let settingsReads = 0; + mockedExecute.mockImplementation((request) => { + if (!request.args[1]?.includes("?license=false")) { + return Effect.succeed(output(mergeRequestJson({}))); + } + settingsReads++; + // A failed read is not held, so the next detail asks again. + return Effect.succeed( + output(settingsReads === 1 ? "invalid json" : '{"merge_method":"merge"}'), + ); + }); + const provider = yield* GitLabPullRequestProvider.make; + assert.isDefined(provider.getChangeRequestSummary); + const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; + + expect((yield* provider.getChangeRequestSummary(input)).title).toBe("Merge request 7"); + expect(settingsReads).toBe(0); + expect((yield* Effect.flip(provider.getChangeRequest(input))).reason).toBe("failed"); + yield* provider.getChangeRequest(input); + yield* provider.getChangeRequest(input); + + expect(settingsReads).toBe(2); + expect(mockedExecute).toHaveBeenCalledTimes(6); + }), + ); }); diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts index a25d5314ef4d..011a332cfdde 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts @@ -1,5 +1,7 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; +import * as Cache from "effect/Cache"; import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; import type { PullRequestCapabilities, PullRequestReaction, @@ -102,6 +104,16 @@ export function gitLabProviderFailure( export const make = Effect.gen(function* () { const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + // How a project lets its merge requests land is a setting, which moves far more slowly than + // the detail every refresh was reading it alongside, and is only returned with that fresh read. + const mergeSettings = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", repository = ""] = key.split("\0"); + return cli.getProjectMergeCapabilities({ cwd, repository }); + }, + { capacity: 32, timeToLive: (exit) => (Exit.isSuccess(exit) ? "5 minutes" : 0) }, + ); + const fail = (operation: string) => (error: GitLabPullRequestCli.GitLabPullRequestCliError) => new PullRequestProviderError({ provider: SourceControlProviderKind.make("gitlab"), @@ -143,11 +155,17 @@ export const make = Effect.gen(function* () { Effect.mapError(fail("getChangeRequestChecks")), ), + // Hover cards and linked rows do not use the project's merge settings. + getChangeRequestPreview: (input) => + cli.getMergeRequestDetail(input).pipe(Effect.mapError(fail("getChangeRequestPreview"))), + getChangeRequestSummary: (input) => + cli.getMergeRequestDetail(input).pipe(Effect.mapError(fail("getChangeRequestSummary"))), + getChangeRequest: (input) => Effect.all( [ cli.getMergeRequestDetail(input), - cli.getProjectMergeCapabilities({ cwd: input.cwd, repository: input.repository }), + Cache.get(mergeSettings, `${input.cwd}\0${input.repository}`), ], { concurrency: 2 }, ).pipe(