From 2bf9fc103aa9764f629ccb434a9dc4b0535ef60a Mon Sep 17 00:00:00 2001 From: Alex Vallone <39172404+amv146@users.noreply.github.com> Date: Mon, 2 Nov 2026 19:19:19 -0800 Subject: [PATCH 1/4] perf(azure-devops): opening a pull request no longer reads it twice Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AzureDevOpsPullRequestProvider.test.ts | 29 +++++++++++ .../server/AzureDevOpsPullRequestProvider.ts | 48 +++++++++++-------- 2 files changed, 56 insertions(+), 21 deletions(-) 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 345d74de84ba1..3dbf757e05ee0 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts @@ -187,6 +187,35 @@ describe("getChangeRequest", () => { expect(detail.changedFiles).toBe(0); }), ); + + it.effect("reads the pull request once for its detail, diff and conversation", () => + 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; + }), + 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([]), + }), + ), + ); + const input = { cwd: "/w", repository: "acme/web", host: "dev.azure.com", number: 7 }; + + yield* provider.getChangeRequest(input); + yield* provider.getDiff(input); + yield* provider.getChangeRequestActivity(input); + + expect(reads).toBe(1); + }), + ); }); describe("getDiff reads", () => { diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index dbd9ae94875bc..f30c463c08a62 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -160,29 +160,34 @@ 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 locationOf = (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( + const readPullRequest = (input: { readonly cwd: string; readonly number: number }) => + cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( Effect.map((pullRequest) => { const location = pullRequest.location; - if (location === null) return null; + 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 location; + return pullRequest; }), ); + + const locationOf = (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 readPullRequest(input).pipe(Effect.map((pullRequest) => pullRequest.location)); }; const diffScope = Effect.fn("AzureDevOpsPullRequestProvider.diffScope")(function* (input: { @@ -294,7 +299,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, @@ -308,13 +313,14 @@ 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), + ), 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 @@ -358,15 +364,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( From 2cd9393d9a7bd297d94a79ee79bf60b6bed4021d Mon Sep 17 00:00:00 2001 From: Alex Vallone Date: Sun, 8 Nov 2026 11:48:16 -0800 Subject: [PATCH 2/4] test(azure-devops): cover location cache seeding across PR reads Use a named Effect reader and preserve live detail reads and bounded recency behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AzureDevOpsPullRequestProvider.test.ts | 141 +++++++++++------- .../server/AzureDevOpsPullRequestProvider.ts | 30 ++-- 2 files changed, 102 insertions(+), 69 deletions(-) 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 3dbf757e05ee0..7fa98a4316873 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"; @@ -188,33 +188,51 @@ describe("getChangeRequest", () => { }), ); - it.effect("reads the pull request once for its detail, diff and conversation", () => - 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; - }), - 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([]), - }), - ), - ); - const input = { cwd: "/w", repository: "acme/web", host: "dev.azure.com", number: 7 }; - - yield* provider.getChangeRequest(input); - yield* provider.getDiff(input); - yield* provider.getChangeRequestActivity(input); + it.effect.each([ + "getChangeRequest", + "getChangeRequestSummary", + "getChangeRequestPreview", + ] 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([]), + }), + ), + ); + const input = { cwd: "/w", repository: "acme/web", host: "dev.azure.com", number: 7 }; + assert.isDefined(provider.getDiffFileContents); + assert.isDefined(provider.getFileRevisions); + + expect((yield* provider[method](input)).title).toBe("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(reads).toBe(1); - }), + expect((yield* provider[method](input)).title).toBe("Read 2"); + expect(reads).toBe(2); + }), ); }); @@ -342,37 +360,52 @@ 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 }); - 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* provider.getChangeRequestSummary({ + 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 f30c463c08a62..4b1786be2a679 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -163,21 +163,21 @@ export const make = Effect.gen(function* () { // PR locations are immutable; every pull request read records one so later reads can skip it. const locations = new Map(); - const readPullRequest = (input: { readonly cwd: string; readonly number: number }) => - cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( - Effect.map((pullRequest) => { - 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 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}`; From b45433a3bbbdff77e20e1f7c07fda566f14d120e Mon Sep 17 00:00:00 2001 From: Alex Vallone Date: Sun, 8 Nov 2026 11:57:15 -0800 Subject: [PATCH 3/4] fix(azure-devops): narrow optional provider readers in cache tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/server/AzureDevOpsPullRequestProvider.test.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) 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 7fa98a4316873..86e003e8787ac 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts @@ -215,10 +215,12 @@ describe("getChangeRequest", () => { ), ); 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* provider[method](input)).title).toBe("Read 1"); + expect((yield* readPullRequest(input)).title).toBe("Read 1"); yield* provider.getDiff(input); yield* provider.getDiffFileContents({ ...input, @@ -230,7 +232,7 @@ describe("getChangeRequest", () => { yield* provider.getChangeRequestActivity(input); expect(reads).toBe(1); - expect((yield* provider[method](input)).title).toBe("Read 2"); + expect((yield* readPullRequest(input)).title).toBe("Read 2"); expect(reads).toBe(2); }), ); @@ -266,6 +268,7 @@ describe("getDiff reads", () => { const readDiff = (number: number) => provider.getDiff({ cwd: "/w", repository: "acme/web", host: "dev.azure.com", number }); + assert.isDefined(provider.getChangeRequestSummary); yield* Effect.all([readDiff(7), readDiff(8)], { concurrency: 2 }); From 8646a41ae98b95d94fa3681521df85ca8b3d62a9 Mon Sep 17 00:00:00 2001 From: Alex Vallone Date: Sun, 8 Nov 2026 12:01:06 -0800 Subject: [PATCH 4/4] fix(azure-devops): guard the cache eviction summary reader Move the assertion from the unrelated concurrency test to the summary reader used by the eviction regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/server/AzureDevOpsPullRequestProvider.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 86e003e8787ac..eda32737b8107 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts @@ -268,8 +268,6 @@ describe("getDiff reads", () => { const readDiff = (number: number) => provider.getDiff({ cwd: "/w", repository: "acme/web", host: "dev.azure.com", number }); - assert.isDefined(provider.getChangeRequestSummary); - yield* Effect.all([readDiff(7), readDiff(8)], { concurrency: 2 }); expect(peakInFlight).toBeLessThanOrEqual(MAX_DIFF_SPAWNS); @@ -389,6 +387,8 @@ describe("what one diff slice spends", () => { 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) { @@ -396,7 +396,7 @@ describe("what one diff slice spends", () => { if (method === "diff") { yield* readDiff(HOT); } else { - yield* provider.getChangeRequestSummary({ + yield* readSummary({ cwd: "/w", repository: "acme/web", host: "dev.azure.com",