diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 3850ed63a7b114d5b5cfb9a7f69511652b789003..370967c0fb29c471545c6b67c51bc8e37d89ef4d 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -309,6 +309,57 @@ it.effect("shares detail and activity for a minute, and for ten once merged", () }), ); +it.effect("re-reads an open pull request after a turn, but not a merged one", () => + Effect.gen(function* () { + const reads: Array = []; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/w", repository: "acme/web" })], + providers: [ + fakeProvider(SourceControlProviderKind.make("github"), { + getChangeRequest: ({ number }) => + Effect.sync(() => { + reads.push(`detail #${number}`); + return { + ...hostedChangeRequest("Description"), + number, + state: number === 1 ? ("open" as const) : ("merged" as const), + }; + }), + getChangeRequestActivity: ({ number }) => + Effect.sync(() => { + reads.push(`activity #${number}`); + return { + comments: [], + commentCount: 0, + commentsTruncated: false, + reviewThreads: [], + commits: [], + }; + }), + }), + ], + }); + // What every client showing #1 (open) and #2 (merged) does on each refresh signal. + const merged = { projectId: "p1" as ProjectId, repository: "acme/web", number: 2 }; + const reread = Effect.forEach([1, 2], (number) => { + const ref = { ...merged, number }; + return Effect.andThen(service.detail(ref), service.activity(ref)); + }).pipe( + Effect.andThen(Effect.yieldNow), + Effect.andThen(Effect.sync(() => reads.splice(0).toSorted())), + ); + + assert.deepStrictEqual(yield* reread, ["activity #1", "activity #2", "detail #1", "detail #2"]); + for (let turn = 0; turn < 3; turn++) { + yield* service.refreshAfterTurn(merged.projectId); + assert.deepStrictEqual(yield* reread, ["activity #1", "detail #1"]); + } + // The refresh button still reads a merged pull request again. + yield* service.invalidate({ reference: merged }); + assert.deepStrictEqual(yield* reread, ["activity #2", "detail #2"]); + }), +); + it.effect("does not wait for an in-flight detail read to display a preview", () => Effect.gen(function* () { const detailStarted = yield* Deferred.make(); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index d57934f10a1678ab7a6404de85c0bc2080974aad..dd60462d5f8c323201577a6f35ff4cc28a35d4ee 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -2800,11 +2800,28 @@ export const make = Effect.gen(function* () { ref.repository.toLowerCase(), ref.number, ]); - const refEpoch = (ref: PullRequestRef) => - Math.max( - projectEpochs.get(ref.projectId) ?? projectEpochFloor, - refEpochs.get(refScope(ref)) ?? 0, + /** + * The project epoch a pull request was first read merged under. A merged pull request cannot + * change again, so a turn in its project no longer moves its key; otherwise every reader showing + * it would read the host again after every turn. A refresh or a mutation still moves it. + */ + const mergedEpochs = new Map(); + const refEpoch = (ref: PullRequestRef) => { + const scope = refScope(ref); + return Math.max( + mergedEpochs.get(scope) ?? projectEpochs.get(ref.projectId) ?? projectEpochFloor, + refEpochs.get(scope) ?? 0, ); + }; + const noteMerged = (ref: PullRequestRef) => { + const scope = refScope(ref); + if (mergedEpochs.has(scope)) return; + if (mergedEpochs.size >= REF_EPOCH_CAPACITY) { + const oldest = mergedEpochs.keys().next().value; + if (oldest !== undefined) mergedEpochs.delete(oldest); + } + mergedEpochs.set(scope, projectEpochs.get(ref.projectId) ?? projectEpochFloor); + }; // Keys carry the reference back out of the cache loader, so the slot layout is shared with // `refOfCacheKey` rather than read positionally at every loader. const refCacheKey = (ref: CredentialRef) => @@ -3213,6 +3230,7 @@ export const make = Effect.gen(function* () { // regression and never ask the host again. const read = Cache.get(detailCache, key).pipe( Effect.tap((value) => { + if (value.state === "merged") noteMerged(input); const summary = summaryFromDetail(value, lastGoodSummary.peek(key)); return noteDetailReading(input, summary).pipe( Effect.andThen(