| author |  Theo Browne <me@t3.gg> 2026-10-14 02:36:46-07:00 |
| committer |  GitHub <noreply@github.com> 2026-10-14 02:36:46-07:00 |
| log | 06a84de3bd613b00f6a1fc61f7f71ea26534ef09 |
| tree | 6ce0bc28a5f0a0d830f927da1fa51df6c5863d1e |
| parent | 7ee2839cb58263742301895a6be826505daa64ea |
| signature | Signed by PGP key B5690EEEBB952194 |
perf(server): turns no longer re-read merged PRs from GitHub (#19344)
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>2 files changed, 73 insertions(+), 4 deletions(-)
apps
server
src
apps/server/src/pullRequest/PullRequestService.test.ts+51
| ... | @@ -309,6 +309,57 @@ it.effect("shares detail and activity for a minute, and for ten once merged", () | ... | @@ -309,6 +309,57 @@ it.effect("shares detail and activity for a minute, and for ten once merged", () |
| 309 | }), | 309 | }), |
| 310 | ); | 310 | ); |
| 311 | 311 | ||
| 312 | it.effect("re-reads an open pull request after a turn, but not a merged one", () => | ||
| 313 | Effect.gen(function* () { | ||
| 314 | const reads: Array<string> = []; | ||
| 315 | const service = yield* makeService({ | ||
| 316 | projects: [project({ id: "p1", title: "web", workspaceRoot: "/w", repository: "acme/web" })], | ||
| 317 | providers: [ | ||
| 318 | fakeProvider(SourceControlProviderKind.make("github"), { | ||
| 319 | getChangeRequest: ({ number }) => | ||
| 320 | Effect.sync(() => { | ||
| 321 | reads.push(`detail #${number}`); | ||
| 322 | return { | ||
| 323 | ...hostedChangeRequest("Description"), | ||
| 324 | number, | ||
| 325 | state: number === 1 ? ("open" as const) : ("merged" as const), | ||
| 326 | }; | ||
| 327 | }), | ||
| 328 | getChangeRequestActivity: ({ number }) => | ||
| 329 | Effect.sync(() => { | ||
| 330 | reads.push(`activity #${number}`); | ||
| 331 | return { | ||
| 332 | comments: [], | ||
| 333 | commentCount: 0, | ||
| 334 | commentsTruncated: false, | ||
| 335 | reviewThreads: [], | ||
| 336 | commits: [], | ||
| 337 | }; | ||
| 338 | }), | ||
| 339 | }), | ||
| 340 | ], | ||
| 341 | }); | ||
| 342 | // What every client showing #1 (open) and #2 (merged) does on each refresh signal. | ||
| 343 | const merged = { projectId: "p1" as ProjectId, repository: "acme/web", number: 2 }; | ||
| 344 | const reread = Effect.forEach([1, 2], (number) => { | ||
| 345 | const ref = { ...merged, number }; | ||
| 346 | return Effect.andThen(service.detail(ref), service.activity(ref)); | ||
| 347 | }).pipe( | ||
| 348 | Effect.andThen(Effect.yieldNow), | ||
| 349 | Effect.andThen(Effect.sync(() => reads.splice(0).toSorted())), | ||
| 350 | ); | ||
| 351 | |||
| 352 | assert.deepStrictEqual(yield* reread, ["activity #1", "activity #2", "detail #1", "detail #2"]); | ||
| 353 | for (let turn = 0; turn < 3; turn++) { | ||
| 354 | yield* service.refreshAfterTurn(merged.projectId); | ||
| 355 | assert.deepStrictEqual(yield* reread, ["activity #1", "detail #1"]); | ||
| 356 | } | ||
| 357 | // The refresh button still reads a merged pull request again. | ||
| 358 | yield* service.invalidate({ reference: merged }); | ||
| 359 | assert.deepStrictEqual(yield* reread, ["activity #2", "detail #2"]); | ||
| 360 | }), | ||
| 361 | ); | ||
| 362 | |||
| 312 | it.effect("does not wait for an in-flight detail read to display a preview", () => | 363 | it.effect("does not wait for an in-flight detail read to display a preview", () => |
| 313 | Effect.gen(function* () { | 364 | Effect.gen(function* () { |
| 314 | const detailStarted = yield* Deferred.make<void>(); | 365 | const detailStarted = yield* Deferred.make<void>(); |
apps/server/src/pullRequest/PullRequestService.ts+22-4
| ... | @@ -2800,11 +2800,28 @@ export const make = Effect.gen(function* () { | ... | @@ -2800,11 +2800,28 @@ export const make = Effect.gen(function* () { |
| 2800 | ref.repository.toLowerCase(), | 2800 | ref.repository.toLowerCase(), |
| 2801 | ref.number, | 2801 | ref.number, |
| 2802 | ]); | 2802 | ]); |
| 2803 | const refEpoch = (ref: PullRequestRef) => | 2803 | /** |
| 2804 | Math.max( | 2804 | * The project epoch a pull request was first read merged under. A merged pull request cannot |
| 2805 | projectEpochs.get(ref.projectId) ?? projectEpochFloor, | 2805 | * change again, so a turn in its project no longer moves its key; otherwise every reader showing |
| 2806 | refEpochs.get(refScope(ref)) ?? 0, | 2806 | * it would read the host again after every turn. A refresh or a mutation still moves it. |
| 2807 | */ | ||
| 2808 | const mergedEpochs = new Map<string, number>(); | ||
| 2809 | const refEpoch = (ref: PullRequestRef) => { | ||
| 2810 | const scope = refScope(ref); | ||
| 2811 | return Math.max( | ||
| 2812 | mergedEpochs.get(scope) ?? projectEpochs.get(ref.projectId) ?? projectEpochFloor, | ||
| 2813 | refEpochs.get(scope) ?? 0, | ||
| 2807 | ); | 2814 | ); |
| 2815 | }; | ||
| 2816 | const noteMerged = (ref: PullRequestRef) => { | ||
| 2817 | const scope = refScope(ref); | ||
| 2818 | if (mergedEpochs.has(scope)) return; | ||
| 2819 | if (mergedEpochs.size >= REF_EPOCH_CAPACITY) { | ||
| 2820 | const oldest = mergedEpochs.keys().next().value; | ||
| 2821 | if (oldest !== undefined) mergedEpochs.delete(oldest); | ||
| 2822 | } | ||
| 2823 | mergedEpochs.set(scope, projectEpochs.get(ref.projectId) ?? projectEpochFloor); | ||
| 2824 | }; | ||
| 2808 | // Keys carry the reference back out of the cache loader, so the slot layout is shared with | 2825 | // Keys carry the reference back out of the cache loader, so the slot layout is shared with |
| 2809 | // `refOfCacheKey` rather than read positionally at every loader. | 2826 | // `refOfCacheKey` rather than read positionally at every loader. |
| 2810 | const refCacheKey = (ref: CredentialRef) => | 2827 | const refCacheKey = (ref: CredentialRef) => |
| ... | @@ -3213,6 +3230,7 @@ export const make = Effect.gen(function* () { | ... | @@ -3213,6 +3230,7 @@ export const make = Effect.gen(function* () { |
| 3213 | // regression and never ask the host again. | 3230 | // regression and never ask the host again. |
| 3214 | const read = Cache.get(detailCache, key).pipe( | 3231 | const read = Cache.get(detailCache, key).pipe( |
| 3215 | Effect.tap((value) => { | 3232 | Effect.tap((value) => { |
| 3233 | if (value.state === "merged") noteMerged(input); | ||
| 3216 | const summary = summaryFromDetail(value, lastGoodSummary.peek(key)); | 3234 | const summary = summaryFromDetail(value, lastGoodSummary.peek(key)); |
| 3217 | return noteDetailReading(input, summary).pipe( | 3235 | return noteDetailReading(input, summary).pipe( |
| 3218 | Effect.andThen( | 3236 | Effect.andThen( |