diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts index 1a1a009bf2fb1..3ee1dbafb56ce 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from "@effect/vitest"; +import { assert, describe, expect, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; @@ -214,6 +214,62 @@ describe("getChangeRequest", () => { }), ); + it.effect.each([ + "getChangeRequest", + "getChangeRequestSummary", + "getChangeRequestPreview", + "getChangeRequestChecks", + ] as const)( + "%s seeds the location for diff, file and conversation reads without caching live details", + (method) => + Effect.gen(function* () { + let reads = 0; + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({ + getPullRequest: () => + Effect.sync(() => { + reads += 1; + return { ...PULL_REQUEST, title: `Read ${reads}` }; + }), + listIterations: () => Effect.succeed([ITERATION]), + listIterationChanges: () => + Effect.succeed({ changes: [change("a.ts")], truncated: false }), + readItemContent: () => + Effect.succeed({ contents: side("new", 2, 4), isBinary: false }), + listThreads: () => Effect.succeed([]), + listPolicyEvaluations: () => Effect.succeed([]), + listPullRequestStatuses: () => Effect.succeed([]), + }), + ), + ); + const input = { cwd: "/w", repository: "acme/web", host: "dev.azure.com", number: 7 }; + const readPullRequest = provider[method]; + assert.isDefined(readPullRequest); + assert.isDefined(provider.getDiffFileContents); + assert.isDefined(provider.getFileRevisions); + + expect(yield* readPullRequest(input)).toMatchObject( + method === "getChangeRequestChecks" ? { state: "open", checks: [] } : { title: "Read 1" }, + ); + yield* provider.getDiff(input); + yield* provider.getDiffFileContents({ + ...input, + oldPath: "a.ts", + newPath: "a.ts", + changeType: "change", + }); + yield* provider.getFileRevisions({ ...input, paths: ["a.ts"] }); + yield* provider.getChangeRequestActivity(input); + expect(reads).toBe(1); + + expect(yield* readPullRequest(input)).toMatchObject( + method === "getChangeRequestChecks" ? { state: "open", checks: [] } : { title: "Read 2" }, + ); + expect(reads).toBe(2); + }), + ); + it.effect("carries the policy checks, and still answers when they cannot be read", () => Effect.gen(function* () { let policyReadFails = false; @@ -337,7 +393,6 @@ describe("getDiff reads", () => { const readDiff = (number: number) => provider.getDiff({ cwd: "/w", repository: "acme/web", host: "dev.azure.com", number }); - yield* Effect.all([readDiff(7), readDiff(8)], { concurrency: 2 }); expect(peakInFlight).toBeLessThanOrEqual(MAX_DIFF_SPAWNS); @@ -431,37 +486,54 @@ describe("what one diff slice spends", () => { expect(read.slice.nextCursor).toBe(`${ITERATION.id}:4`); }), ); - it.effect("keeps the pull request being read, not the one looked up first", () => - Effect.gen(function* () { - const HOT = 7; - const readsOf = new Map(); + it.effect.each(["diff", "summary"] as const)( + "keeps the pull request being read by %s, not the one looked up first", + (method) => + Effect.gen(function* () { + const HOT = 7; + const readsOf = new Map(); - const provider = yield* make.pipe( - Effect.provide( - Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({ - getPullRequest: (input) => - Effect.sync(() => { - readsOf.set(input.number, (readsOf.get(input.number) ?? 0) + 1); - return { ...PULL_REQUEST, number: input.number }; - }), - listIterations: () => Effect.succeed([ITERATION]), - listIterationChanges: () => - Effect.succeed({ changes: [change("a.ts")], truncated: false }), - readItemContent: () => Effect.succeed({ contents: side("new", 2, 4), isBinary: false }), - }), - ), - ); + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({ + getPullRequest: (input) => + Effect.sync(() => { + readsOf.set(input.number, (readsOf.get(input.number) ?? 0) + 1); + return { ...PULL_REQUEST, number: input.number }; + }), + listIterations: () => Effect.succeed([ITERATION]), + listIterationChanges: () => + Effect.succeed({ changes: [change("a.ts")], truncated: false }), + readItemContent: () => + Effect.succeed({ contents: side("new", 2, 4), isBinary: false }), + }), + ), + ); - const readDiff = (number: number) => - provider.getDiff({ cwd: "/w", repository: "acme/web", host: "dev.azure.com", number }); + const readDiff = (number: number) => + provider.getDiff({ cwd: "/w", repository: "acme/web", host: "dev.azure.com", number }); + const readSummary = provider.getChangeRequestSummary; + assert.isDefined(readSummary); - yield* readDiff(HOT); - for (let filled = 0; filled < LOCATION_CACHE_CAPACITY; filled += 1) { - yield* readDiff(HOT + 1 + filled); yield* readDiff(HOT); - } + for (let filled = 0; filled < LOCATION_CACHE_CAPACITY; filled += 1) { + yield* readDiff(HOT + 1 + filled); + if (method === "diff") { + yield* readDiff(HOT); + } else { + yield* readSummary({ + cwd: "/w", + repository: "acme/web", + host: "dev.azure.com", + number: HOT, + }); + } + } - expect(readsOf.get(HOT)).toBe(1); - }), + yield* readDiff(HOT); + expect(readsOf.get(HOT)).toBe(method === "diff" ? 1 : LOCATION_CACHE_CAPACITY + 1); + yield* readDiff(HOT + 1); + expect(readsOf.get(HOT + 1)).toBe(2); + }), ); }); diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index 8df4e53237b52..fdd14db3d16c3 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -186,9 +186,25 @@ export const make = Effect.gen(function* () { const EMPTY_DIFF_SLICE: ProviderDiffSlice = { patch: "", truncated: false, nextCursor: null }; - // PR locations are immutable; caching avoids full PR reads for viewed-file checks. + // PR locations are immutable; every pull request read records one so later reads can skip it. const locations = new Map(); + const readPullRequest = Effect.fn("AzureDevOpsPullRequestProvider.readPullRequest")( + function* (input: { readonly cwd: string; readonly number: number }) { + const pullRequest = yield* cli.getPullRequest({ cwd: input.cwd, number: input.number }); + const location = pullRequest.location; + if (location === null) return pullRequest; + const key = `${input.cwd} ${input.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, location); + return pullRequest; + }, + ); + const locationOf = (input: { readonly cwd: string; readonly number: number }) => { const key = `${input.cwd} ${input.number}`; const held = locations.get(key); @@ -197,18 +213,7 @@ export const make = Effect.gen(function* () { 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 readPullRequest(input).pipe(Effect.map((pullRequest) => pullRequest.location)); }; const diffScope = Effect.fn("AzureDevOpsPullRequestProvider.diffScope")(function* (input: { @@ -320,7 +325,7 @@ export const make = Effect.gen(function* () { ), getChangeRequestPreview: (input) => - cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( + readPullRequest(input).pipe( Effect.mapError(fail("getChangeRequestPreview")), Effect.map((pullRequest) => ({ number: pullRequest.number, @@ -334,16 +339,17 @@ export const make = Effect.gen(function* () { ), getChangeRequestSummary: (input) => - cli - .getPullRequest({ cwd: input.cwd, number: input.number }) - .pipe(Effect.mapError(fail("getChangeRequestSummary")), Effect.map(toSummary)), + readPullRequest(input).pipe( + Effect.mapError(fail("getChangeRequestSummary")), + Effect.map(toSummary), + ), getChangeRequestChecks: Effect.fn("AzureDevOpsPullRequestProvider.getChangeRequestChecks")( function* (input) { // Policies need only the number, so they read while the pull request is still loading. const [{ pullRequest, statuses }, evaluations] = yield* Effect.all( [ - cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( + readPullRequest(input).pipe( Effect.flatMap((pullRequest) => listStatuses({ cwd: input.cwd, @@ -363,7 +369,7 @@ export const make = Effect.gen(function* () { getChangeRequest: Effect.fn("AzureDevOpsPullRequestProvider.getChangeRequest")( function* (input) { - const pullRequest = yield* cli.getPullRequest({ cwd: input.cwd, number: input.number }); + const pullRequest = yield* readPullRequest(input); const location = pullRequest.location; const iterations = location === null @@ -425,15 +431,15 @@ export const make = Effect.gen(function* () { ), getChangeRequestActivity: (input) => - cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( + locationOf(input).pipe( Effect.mapError(fail("getChangeRequestActivity")), - Effect.flatMap((pullRequest) => - (pullRequest.location === null + Effect.flatMap((location) => + (location === null ? Effect.succeed({ comments: [], truncated: true }) : cli .listThreads({ cwd: input.cwd, - location: pullRequest.location, + location, number: input.number, }) .pipe(