From d878b131b51d8b0eb4e35e8cffe36143f1f0c6cd Mon Sep 17 00:00:00 2001 From: Lars Nieuwenhuis <35393046+lnieuwenhuis@users.noreply.github.com> Date: Sat, 5 Sep 2026 00:23:51 +0200 Subject: [PATCH 1/8] fix(web): pull request panel polls go around the server's hold The panel's automatic refresh (five-minute interval and return-to-window read) now invalidates the pull request on the server before re-reading its detail, the same way the manual Refresh action already does. An ordinary detail read is answered from the server's hold while it refreshes behind the answer, so each poll showed the previous poll's data and a passively watched panel never caught an external title or check change. The invalidation is scoped to that one pull request (bumpRefEpoch). --- .../pullRequest/PullRequestService.test.ts | 62 +++++++++++++++++++ .../pullRequest/PullRequestDetailPanel.tsx | 29 +++++---- 2 files changed, 79 insertions(+), 12 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 43ba73d325ef..cf4c068983ab 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3228,6 +3228,68 @@ it.effect("answers a known pull request immediately while the host refreshes", ( }), ); +it.effect("an invalidated detail read sees an external change a plain re-read holds back", () => + Effect.gen(function* () { + const gate = yield* Deferred.make(); + let calls = 0; + let hostTitle = "old title"; + let hostChecks: ReadonlyArray<{ readonly name: string; readonly status: "success" }> = []; + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getChangeRequest: () => + Effect.gen(function* () { + calls += 1; + // The plain re-read's background refresh stalls here, so the hold stays old + // while the invalidated poll read runs past it. + if (calls === 2) yield* Deferred.await(gate); + return { + ...hostedChangeRequest("polled body", 4), + title: hostTitle, + checks: hostChecks.map((check) => ({ + name: check.name, + status: check.status, + description: null, + url: null, + })), + }; + }), + }), + ], + }); + + const first = yield* service.detail(reference); + assert.strictEqual(first.title, "old title"); + assert.deepStrictEqual(first.checks, []); + + hostTitle = "new title"; + hostChecks = [{ name: "ci", status: "success" }]; + yield* TestClock.adjust("16 seconds"); + + // The old poll path: a plain re-read answers from the hold while refreshing behind it. + const second = yield* service.detail(reference); + assert.strictEqual(second.title, "old title"); + assert.deepStrictEqual(second.checks, []); + yield* Effect.yieldNow; + assert.strictEqual(calls, 2); + + // The poll path the panel now takes: invalidate first so the re-read misses the hold, + // even with that background refresh still in flight. + yield* service.invalidate({ reference }); + const polled = yield* service.detail(reference); + assert.strictEqual(polled.title, "new title"); + assert.deepStrictEqual( + polled.checks.map((check) => check.name), + ["ci"], + ); + + yield* Deferred.succeed(gate, undefined); + yield* Effect.yieldNow; + }), +); + it.effect("does not ask the host again for a linked summary it already holds", () => Effect.gen(function* () { let calls = 0; diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 4a3af4b02c52..f6d8000045bb 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -713,22 +713,27 @@ export function PullRequestDetailPanel({ state: resolvedCoreDetail.state, }); }, [onStateChange, resolvedCoreDetail]); + // Both reads below go around the server's cache rather than through it. An ordinary read is + // answered from what the server already holds while it refreshes behind the answer, which is + // right for a reopen and wrong for a poll: the point of a poll is what that refresh brings back, + // and nothing pushes it to a panel already told the old answer, so a poll served from the hold + // shows the previous poll's data for good. The invalidation goes first so the re-reads miss + // that cache; if it fails, the reads still run and at worst answer from it. + const invalidate = useAtomCommand(pullRequestEnvironment.invalidate, { reportFailure: false }); // Core detail is cheap enough to re-read while this stays open. Activity is heavier, so the // revision effect above reads it only after this same pull request reports a change. Keyed by // the pull request rather than by the panel, because this one panel shows a different pull // request every time it is opened. - useLiveRefresh( - () => { - detailQuery.refresh(); - setRefreshToken((token) => token + 1); - }, - { key: `pull-request:${reference.projectId}:${reference.repository}#${reference.number}` }, - ); - // The button, on the other hand, goes around the server's cache rather than through it: it is - // the answer for a reader who can see that what they are looking at is behind. The - // invalidation goes first so the re-reads miss that cache; if it fails, the reads still run - // and at worst answer from it. - const invalidate = useAtomCommand(pullRequestEnvironment.invalidate, { reportFailure: false }); + const refreshDetailFromHost = useCallback(async () => { + await invalidate({ environmentId, input: { reference } }); + detailQuery.refresh(); + setRefreshToken((token) => token + 1); + }, [detailQuery.refresh, environmentId, invalidate, reference]); + useLiveRefresh(() => void refreshDetailFromHost(), { + key: `pull-request:${reference.projectId}:${reference.repository}#${reference.number}`, + }); + // The button is the answer for a reader who can see that what they are looking at is behind, + // so it reads everything: the detail, the activity, and through the refresh token, the diff. const refreshFromHost = useCallback(async () => { await invalidate({ environmentId, input: { reference } }); refreshDetail(); From 103ffcd9cfa73ee481be6307558d9912a804729a Mon Sep 17 00:00:00 2001 From: Lars Nieuwenhuis <35393046+lnieuwenhuis@users.noreply.github.com> Date: Sat, 5 Sep 2026 01:03:43 +0200 Subject: [PATCH 2/8] fix(pr): poll invalidates detail only so the diff keeps stale-while-revalidate --- .../pullRequest/PullRequestService.test.ts | 57 +++++++++++++++++++ .../src/pullRequest/PullRequestService.ts | 27 ++++++--- .../pullRequest/PullRequestDetailPanel.tsx | 21 ++++--- packages/contracts/src/pullRequest.ts | 3 + 4 files changed, 92 insertions(+), 16 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index cf4c068983ab..042ed0493742 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3290,6 +3290,63 @@ it.effect("an invalidated detail read sees an external change a plain re-read ho }), ); +it.effect("a detail-scoped invalidate refreshes detail without stranding the held diff", () => + Effect.gen(function* () { + const diffGate = yield* Deferred.make(); + let detailCalls = 0; + let diffCalls = 0; + let hostTitle = "old title"; + let hostPatch = "old patch"; + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getChangeRequest: () => + Effect.sync(() => { + detailCalls += 1; + return { ...hostedChangeRequest("polled body", 4), title: hostTitle }; + }), + getDiff: () => + Effect.gen(function* () { + diffCalls += 1; + // The stale-while-revalidate background refresh stalls here, so a held diff + // must answer from its snapshot rather than wait on the host. + if (diffCalls === 2) yield* Deferred.await(diffGate); + return { patch: hostPatch, truncated: false, nextCursor: null }; + }), + }), + ], + }); + + const firstDetail = yield* service.detail(reference); + assert.strictEqual(firstDetail.title, "old title"); + const firstDiff = yield* service.diff(reference); + assert.strictEqual(firstDiff.patch, "old patch"); + assert.strictEqual(diffCalls, 1); + + hostTitle = "new title"; + hostPatch = "new patch"; + // Past the diff cache TTL but inside the stale-while-revalidate window, so a held diff + // answers from its snapshot while refreshing behind it. + yield* TestClock.adjust("61 seconds"); + + // The poll path: detail misses the hold while the diff key — and its hold — is untouched. + yield* service.invalidate({ reference, scope: "detail" }); + const polledDetail = yield* service.detail(reference); + assert.strictEqual(polledDetail.title, "new title"); + assert.strictEqual(detailCalls, 2); + + const polledDiff = yield* service.diff(reference); + assert.strictEqual(polledDiff.patch, "old patch"); + yield* Effect.yieldNow; + assert.strictEqual(diffCalls, 2); + + yield* Deferred.succeed(diffGate, undefined); + yield* Effect.yieldNow; + }), +); + it.effect("does not ask the host again for a linked summary it already holds", () => Effect.gen(function* () { let calls = 0; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index ceba7ce32e04..eec530949cf5 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -2126,23 +2126,34 @@ export const make = Effect.gen(function* () { // epoch strands every entry made under the old one — no enumerating a cache whose keys // (cursors, commits) nothing holds a list of. The counter is shared and monotonic so a // scope re-entering `refEpochs` after eviction can never mint a key an old entry still has. + // Detail and diff ride separate epochs: a poll only needs title/check freshness, so it + // strands the cheap detail hold while the mounted diff keeps its stale-while-revalidate + // path. Mutations and the manual refresh strand both. let epochCounter = 0; let listingsEpoch = 0; let turnRefreshEpoch = 0; const refEpochs = new Map(); + const diffEpochs = new Map(); const REF_EPOCH_CAPACITY = 2_048; const refScope = (ref: PullRequestRef) => `${ref.projectId} ${ref.repository} ${ref.number}`; const refEpoch = (ref: PullRequestRef) => Math.max(turnRefreshEpoch, refEpochs.get(refScope(ref)) ?? 0); + const diffEpoch = (ref: PullRequestRef) => + Math.max(turnRefreshEpoch, diffEpochs.get(refScope(ref)) ?? 0); const refCacheKey = (ref: PullRequestRef) => JSON.stringify([refEpoch(ref), ref.projectId, ref.repository, ref.number]); - const bumpRefEpoch = (ref: PullRequestRef) => { + const bumpMapEpoch = (epochs: Map, ref: PullRequestRef) => { const scope = refScope(ref); - if (!refEpochs.has(scope) && refEpochs.size >= REF_EPOCH_CAPACITY) { - const oldest = refEpochs.keys().next().value; - if (oldest !== undefined) refEpochs.delete(oldest); + if (!epochs.has(scope) && epochs.size >= REF_EPOCH_CAPACITY) { + const oldest = epochs.keys().next().value; + if (oldest !== undefined) epochs.delete(oldest); } - refEpochs.set(scope, ++epochCounter); + epochs.set(scope, ++epochCounter); + }; + const bumpDetailEpoch = (ref: PullRequestRef) => bumpMapEpoch(refEpochs, ref); + const bumpRefEpoch = (ref: PullRequestRef) => { + bumpMapEpoch(refEpochs, ref); + bumpMapEpoch(diffEpochs, ref); }; /** The positional filter slot of a cache key, back as the record `listUncached` takes. */ @@ -2354,7 +2365,7 @@ export const make = Effect.gen(function* () { ); const diff: PullRequestService["Service"]["diff"] = (input) => { const key = JSON.stringify([ - refEpoch(input), + diffEpoch(input), input.projectId, input.repository, input.number, @@ -2396,7 +2407,9 @@ export const make = Effect.gen(function* () { const invalidate: PullRequestService["Service"]["invalidate"] = (input) => { const reference = input.reference; if (reference !== undefined) { - return Effect.sync(() => bumpRefEpoch(reference)); + return Effect.sync(() => + input.scope === "detail" ? bumpDetailEpoch(reference) : bumpRefEpoch(reference), + ); } return Effect.sync(() => { listingsEpoch = ++epochCounter; diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index f6d8000045bb..6d590b962420 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -713,21 +713,24 @@ export function PullRequestDetailPanel({ state: resolvedCoreDetail.state, }); }, [onStateChange, resolvedCoreDetail]); - // Both reads below go around the server's cache rather than through it. An ordinary read is - // answered from what the server already holds while it refreshes behind the answer, which is - // right for a reopen and wrong for a poll: the point of a poll is what that refresh brings back, - // and nothing pushes it to a panel already told the old answer, so a poll served from the hold - // shows the previous poll's data for good. The invalidation goes first so the re-reads miss - // that cache; if it fails, the reads still run and at worst answer from it. + // The refreshes below go around the server's cache rather than through it. An ordinary + // read is answered from what the server already holds while it refreshes behind the answer, + // which is right for a reopen and wrong for a poll: the point of a poll is what that refresh + // brings back, and nothing pushes it to a panel already told the old answer, so a poll served + // from the hold shows the previous poll's data for good. The invalidation goes first so the + // re-reads miss that cache; if it fails, the reads still run and at worst answer from it. + // The poll strands only the detail hold while the button strands detail and diff alike. const invalidate = useAtomCommand(pullRequestEnvironment.invalidate, { reportFailure: false }); // Core detail is cheap enough to re-read while this stays open. Activity is heavier, so the // revision effect above reads it only after this same pull request reports a change. Keyed by // the pull request rather than by the panel, because this one panel shows a different pull - // request every time it is opened. + // request every time it is opened. The poll strands only the detail hold: title and check + // freshness need the detail re-read to miss it, while the mounted Code tab keeps its slices + // and its server stale-while-revalidate diff path instead of waiting on a full host fetch + // every interval and focus. const refreshDetailFromHost = useCallback(async () => { - await invalidate({ environmentId, input: { reference } }); + await invalidate({ environmentId, input: { reference, scope: "detail" } }); detailQuery.refresh(); - setRefreshToken((token) => token + 1); }, [detailQuery.refresh, environmentId, invalidate, reference]); useLiveRefresh(() => void refreshDetailFromHost(), { key: `pull-request:${reference.projectId}:${reference.repository}#${reference.number}`, diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index f766578bb1a3..d9052e0a21cb 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -685,9 +685,12 @@ export type PullRequestListStatsResult = typeof PullRequestListStatsResult.Type; * forgets that one change request's detail and diff; without one it forgets the listings. * A separate request rather than a flag on the reads, so an explicit "refresh" one person * presses is the only thing that spends host requests — every ordinary read shares. + * The detail scope forgets only the detail (and its summary/activity siblings) while leaving + * the diff's stale-while-revalidate hold alone, for polls that only need title/check freshness. */ export const PullRequestInvalidateInput = Schema.Struct({ reference: Schema.optional(PullRequestRef), + scope: Schema.optional(Schema.Literals(["detail", "all"])), }); export type PullRequestInvalidateInput = typeof PullRequestInvalidateInput.Type; From 700908369467957d0eb283c797334ae81561c1b0 Mon Sep 17 00:00:00 2001 From: Lars Nieuwenhuis <35393046+lnieuwenhuis@users.noreply.github.com> Date: Sun, 6 Sep 2026 21:48:22 +0200 Subject: [PATCH 3/8] fix(pr): retain diff revision across metadata invalidation --- .../pullRequest/PullRequestService.test.ts | 22 +++++++++++++++++++ .../src/pullRequest/PullRequestService.ts | 19 +++++++++++----- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 5e00ead1c490..1b51918901e4 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3475,10 +3475,22 @@ it.effect("a changed revision invalidates every held diff page before reloading" const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; const calls: Array = []; let revision = "old"; + let failDetail = false; const service = yield* makeService({ projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], providers: [ fakeProvider("github", { + getChangeRequest: () => + failDetail + ? Effect.fail( + new PullRequestProviderError({ + provider: "github", + operation: "getChangeRequest", + reason: "failed", + detail: "HTTP 503", + }), + ) + : Effect.succeed(hostedChangeRequest("body", 4)), getDiff: (input) => Effect.sync(() => { calls.push(input.cursor); @@ -3491,6 +3503,7 @@ it.effect("a changed revision invalidates every held diff page before reloading" }), ], }); + yield* service.detail(reference); assert.strictEqual((yield* service.diff(reference)).patch, "old:first"); assert.strictEqual( (yield* service.diff({ ...reference, cursor: "page-2" })).patch, @@ -3506,6 +3519,15 @@ it.effect("a changed revision invalidates every held diff page before reloading" ); assert.deepStrictEqual(calls, [undefined, "page-2"]); + failDetail = true; + yield* Effect.flip(service.detail(reference)); + assert.strictEqual((yield* service.diff(reference)).patch, "old:first"); + assert.strictEqual( + (yield* service.diff({ ...reference, cursor: "page-2" })).patch, + "old:page-2", + ); + assert.deepStrictEqual(calls, [undefined, "page-2"]); + yield* service.invalidate({ reference }); assert.strictEqual((yield* service.diff(reference)).patch, "new:first"); assert.strictEqual( diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index db0505a9198b..0581ed11a0b5 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -2401,19 +2401,28 @@ export const make = Effect.gen(function* () { }, }, ); + const lastGoodDiffRevision = makeLastGoodRead(DIFF_CACHE_CAPACITY); const diff: PullRequestService["Service"]["diff"] = (input) => { + const epoch = diffEpoch(input); + const revisionKey = JSON.stringify([epoch, input.projectId, input.repository, input.number]); + const observedRevision = lastGoodSummary.peek(refCacheKey(input))?.updatedAt; + // Detail invalidation must not erase the revision identifying held diff pages while + // the fresh metadata is pending or fails. Full invalidation changes this key's epoch. + const revision = observedRevision ?? lastGoodDiffRevision.peek(revisionKey) ?? null; + const rememberRevision = + input.commit === undefined && observedRevision !== undefined + ? lastGoodDiffRevision.record(revisionKey, observedRevision) + : Effect.void; const key = JSON.stringify([ - diffEpoch(input), + epoch, input.projectId, input.repository, input.number, input.cursor ?? null, input.commit ?? null, - input.commit === undefined - ? (lastGoodSummary.peek(refCacheKey(input))?.updatedAt ?? null) - : null, + input.commit === undefined ? revision : null, ]); - return staleDiff(key, Cache.get(diffCache, key)); + return rememberRevision.pipe(Effect.andThen(staleDiff(key, Cache.get(diffCache, key)))); }; const listStatsCache = yield* Cache.makeWith( From 15c4cb017d438d5890f7d8af389e20326952e151 Mon Sep 17 00:00:00 2001 From: Lars Nieuwenhuis <35393046+lnieuwenhuis@users.noreply.github.com> Date: Wed, 9 Sep 2026 11:25:16 +0200 Subject: [PATCH 4/8] test(server): cover diff refresh after detail revision changes --- .../pullRequest/PullRequestService.test.ts | 55 ++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 1b51918901e4..2fc2b05dee48 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3470,7 +3470,60 @@ it.effect("a detail-scoped invalidate refreshes detail without stranding the hel }), ); -it.effect("a changed revision invalidates every held diff page before reloading", () => +it.effect("a changed updatedAt reloads every held diff page after detail-only invalidation", () => + Effect.gen(function* () { + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const calls: Array = []; + let updatedAt = "2026-07-02T00:00:00Z"; + let patchVersion = "old"; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getChangeRequest: () => Effect.succeed({ ...hostedChangeRequest("body", 4), updatedAt }), + getDiff: (input) => + Effect.sync(() => { + calls.push(input.cursor); + return { + patch: `${patchVersion}:${input.cursor ?? "first"}`, + truncated: false, + nextCursor: input.cursor ? null : "page-2", + }; + }), + }), + ], + }); + + yield* service.detail(reference); + assert.strictEqual((yield* service.diff(reference)).patch, "old:first"); + assert.strictEqual( + (yield* service.diff({ ...reference, cursor: "page-2" })).patch, + "old:page-2", + ); + + patchVersion = "new"; + yield* service.invalidate({ reference, scope: "detail" }); + yield* service.detail(reference); + assert.strictEqual((yield* service.diff(reference)).patch, "old:first"); + assert.strictEqual( + (yield* service.diff({ ...reference, cursor: "page-2" })).patch, + "old:page-2", + ); + assert.deepStrictEqual(calls, [undefined, "page-2"]); + + updatedAt = "2026-07-03T00:00:00Z"; + yield* service.invalidate({ reference, scope: "detail" }); + assert.strictEqual((yield* service.detail(reference)).updatedAt, updatedAt); + assert.strictEqual((yield* service.diff(reference)).patch, "new:first"); + assert.strictEqual( + (yield* service.diff({ ...reference, cursor: "page-2" })).patch, + "new:page-2", + ); + assert.deepStrictEqual(calls, [undefined, "page-2", undefined, "page-2"]); + }), +); + +it.effect("full invalidation reloads every held diff page after a failed detail refresh", () => Effect.gen(function* () { const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; const calls: Array = []; From 8744c1c47b29faf515ee94b3b08e35ecd203b300 Mon Sep 17 00:00:00 2001 From: Lars Nieuwenhuis <35393046+lnieuwenhuis@users.noreply.github.com> Date: Wed, 9 Sep 2026 11:30:56 +0200 Subject: [PATCH 5/8] fix(server): keep evicted PR cache generations unique --- .../pullRequest/PullRequestService.test.ts | 54 +++++++++++++++++++ .../src/pullRequest/PullRequestService.ts | 13 +++-- 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 2fc2b05dee48..bee24534c983 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3470,6 +3470,60 @@ it.effect("a detail-scoped invalidate refreshes detail without stranding the hel }), ); +for (const afterTurn of [false, true]) { + it.effect( + `evicting invalidation epochs never revives held responses (after turn: ${afterTurn})`, + () => + Effect.gen(function* () { + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + let version = "old"; + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" }), + ], + providers: [ + fakeProvider("github", { + getChangeRequest: () => + Effect.succeed({ ...hostedChangeRequest("body", 4), title: version }), + getDiff: (input) => + Effect.succeed({ + patch: `${version}:${input.cursor ?? "first"}`, + truncated: false, + nextCursor: input.cursor ? null : "page-2", + }), + }), + ], + }); + + assert.strictEqual((yield* service.detail(reference)).title, "old"); + assert.strictEqual((yield* service.diff(reference)).patch, "old:first"); + assert.strictEqual( + (yield* service.diff({ ...reference, cursor: "page-2" })).patch, + "old:page-2", + ); + if (afterTurn) { + yield* service.refreshAfterTurn; + yield* service.detail(reference); + yield* service.diff(reference); + yield* service.diff({ ...reference, cursor: "page-2" }); + } + version = "new"; + yield* service.invalidate({ reference }); + // Fill the bounded epoch maps with other scopes without evicting the held responses. + for (let number = 2; number <= 2_049; number += 1) { + yield* service.invalidate({ reference: { ...reference, number } }); + } + + assert.strictEqual((yield* service.detail(reference)).title, "new"); + assert.strictEqual((yield* service.diff(reference)).patch, "new:first"); + assert.strictEqual( + (yield* service.diff({ ...reference, cursor: "page-2" })).patch, + "new:page-2", + ); + }), + ); +} + it.effect("a changed updatedAt reloads every held diff page after detail-only invalidation", () => Effect.gen(function* () { const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 0581ed11a0b5..e38f385e818f 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -2138,10 +2138,8 @@ export const make = Effect.gen(function* () { const diffEpochs = new Map(); const REF_EPOCH_CAPACITY = 2_048; const refScope = (ref: PullRequestRef) => `${ref.projectId} ${ref.repository} ${ref.number}`; - const refEpoch = (ref: PullRequestRef) => - Math.max(turnRefreshEpoch, refEpochs.get(refScope(ref)) ?? 0); - const diffEpoch = (ref: PullRequestRef) => - Math.max(turnRefreshEpoch, diffEpochs.get(refScope(ref)) ?? 0); + const refEpoch = (ref: PullRequestRef) => mapEpoch(refEpochs, ref); + const diffEpoch = (ref: PullRequestRef) => mapEpoch(diffEpochs, ref); const refCacheKey = (ref: PullRequestRef) => JSON.stringify([refEpoch(ref), ref.projectId, ref.repository, ref.number]); // Counts belong to a PR, not a filtered page. Background reads and filter changes reuse @@ -2165,8 +2163,13 @@ export const make = Effect.gen(function* () { const oldest = epochs.keys().next().value; if (oldest !== undefined) epochs.delete(oldest); } - epochs.set(scope, ++epochCounter); + const epoch = ++epochCounter; + epochs.set(scope, epoch); + return epoch; }; + // Reads reserve an epoch too: returning a default after eviction would revive held keys. + const mapEpoch = (epochs: Map, ref: PullRequestRef) => + Math.max(turnRefreshEpoch, epochs.get(refScope(ref)) ?? bumpMapEpoch(epochs, ref)); const bumpDetailEpoch = (ref: PullRequestRef) => bumpMapEpoch(refEpochs, ref); const bumpRefEpoch = (ref: PullRequestRef) => { bumpMapEpoch(refEpochs, ref); From 63478c9c545c2220dccae67817be41b2fe76609f Mon Sep 17 00:00:00 2001 From: Lars Nieuwenhuis <35393046+lnieuwenhuis@users.noreply.github.com> Date: Wed, 9 Sep 2026 11:41:17 +0200 Subject: [PATCH 6/8] fix(server): retain recently used PR cache epochs --- .../pullRequest/PullRequestService.test.ts | 40 +++++++++++++++++++ .../src/pullRequest/PullRequestService.ts | 10 ++++- 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index bee24534c983..41996b18edad 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3470,6 +3470,46 @@ it.effect("a detail-scoped invalidate refreshes detail without stranding the hel }), ); +it.effect("recently read scopes retain held responses when epoch maps reach capacity", () => + Effect.gen(function* () { + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + let failHost = false; + const failure = new PullRequestProviderError({ + provider: "github", + operation: "read", + reason: "failed", + detail: "HTTP 503", + }); + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getChangeRequest: () => + failHost ? Effect.fail(failure) : Effect.succeed(hostedChangeRequest("held body", 4)), + getDiff: () => + failHost + ? Effect.fail(failure) + : Effect.succeed({ patch: "held patch", truncated: false, nextCursor: null }), + }), + ], + }); + const heldDetail = yield* service.detail(reference); + yield* service.diff(reference); + for (let number = 2; number <= 2_048; number += 1) { + yield* service.invalidate({ reference: { ...reference, number } }); + } + // Reading the active scope keeps its generations newer than the untouched scopes. + yield* service.detail(reference); + yield* service.diff(reference); + yield* service.invalidate({ reference: { ...reference, number: 2_049 } }); + failHost = true; + yield* TestClock.adjust("61 seconds"); + + assert.deepStrictEqual(yield* service.detail(reference), heldDetail); + assert.strictEqual((yield* service.diff(reference)).patch, "held patch"); + }), +); + for (const afterTurn of [false, true]) { it.effect( `evicting invalidation epochs never revives held responses (after turn: ${afterTurn})`, diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index e38f385e818f..4f6db29087bd 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -2164,12 +2164,18 @@ export const make = Effect.gen(function* () { if (oldest !== undefined) epochs.delete(oldest); } const epoch = ++epochCounter; + epochs.delete(scope); epochs.set(scope, epoch); return epoch; }; // Reads reserve an epoch too: returning a default after eviction would revive held keys. - const mapEpoch = (epochs: Map, ref: PullRequestRef) => - Math.max(turnRefreshEpoch, epochs.get(refScope(ref)) ?? bumpMapEpoch(epochs, ref)); + const mapEpoch = (epochs: Map, ref: PullRequestRef) => { + const scope = refScope(ref); + const epoch = epochs.get(scope) ?? bumpMapEpoch(epochs, ref); + epochs.delete(scope); + epochs.set(scope, epoch); + return Math.max(turnRefreshEpoch, epoch); + }; const bumpDetailEpoch = (ref: PullRequestRef) => bumpMapEpoch(refEpochs, ref); const bumpRefEpoch = (ref: PullRequestRef) => { bumpMapEpoch(refEpochs, ref); From c2016838007a60d31ddc5a0734dd7f88a4f5b699 Mon Sep 17 00:00:00 2001 From: Lars Nieuwenhuis <35393046+lnieuwenhuis@users.noreply.github.com> Date: Thu, 10 Sep 2026 10:13:05 +0200 Subject: [PATCH 7/8] fix(web): report failed manual PR refreshes --- .../usePullRequestRefresh.test.tsx | 33 +++++++++++++++++++ .../pullRequest/usePullRequestRefresh.ts | 10 +++++- 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/apps/web/src/components/pullRequest/usePullRequestRefresh.test.tsx b/apps/web/src/components/pullRequest/usePullRequestRefresh.test.tsx index 31a4e9a32c7b..79cd6c0a7d11 100644 --- a/apps/web/src/components/pullRequest/usePullRequestRefresh.test.tsx +++ b/apps/web/src/components/pullRequest/usePullRequestRefresh.test.tsx @@ -199,6 +199,39 @@ describe("mounted pull request refresh sequencing", () => { expect(diffRefreshes()).toBe("0"); }); + it("reports failed manual invalidation without refreshing and allows a successful retry", async () => { + await render(); + const failed = invalidation(); + await act(async () => { + void renderer!.root.findByType("button").props.onClick(); + }); + expect(renderer!.root.findByType("button").props.disabled).toBe(true); + expect(refreshDetail).not.toHaveBeenCalled(); + await failed.fail(); + expect(refreshDetail).not.toHaveBeenCalled(); + expect(diffRefreshes()).toBe("0"); + expect(notify).toHaveBeenCalledExactlyOnceWith({ + type: "error", + title: "The pull request could not be refreshed", + description: "offline", + }); + expect(renderer!.root.findByType("button").props.disabled).toBe(false); + + const retry = invalidation(); + await act(async () => { + void renderer!.root.findByType("button").props.onClick(); + }); + expect(renderer!.root.findByType("button").props.disabled).toBe(true); + expect(refreshDetail).not.toHaveBeenCalled(); + expect(diffRefreshes()).toBe("0"); + await retry.succeed(); + expect(invalidate).toHaveBeenCalledTimes(2); + expect(refreshDetail).toHaveBeenCalledOnce(); + expect(diffRefreshes()).toBe("1"); + expect(notify).toHaveBeenCalledOnce(); + expect(renderer!.root.findByType("button").props.disabled).toBe(false); + }); + it("awaits full invalidation before a page refresh", async () => { await render(); const pending = invalidation(); diff --git a/apps/web/src/components/pullRequest/usePullRequestRefresh.ts b/apps/web/src/components/pullRequest/usePullRequestRefresh.ts index 615c275d0852..d35734d552df 100644 --- a/apps/web/src/components/pullRequest/usePullRequestRefresh.ts +++ b/apps/web/src/components/pullRequest/usePullRequestRefresh.ts @@ -83,7 +83,15 @@ export function usePullRequestRefresh({ const refreshFromHost = useCallback(async () => { setIsInvalidating(true); try { - await invalidate({ environmentId, input: { reference } }); + const result = await invalidate({ environmentId, input: { reference } }); + if (result._tag === "Failure") { + toastManager.add({ + type: "error", + title: "The pull request could not be refreshed", + description: readableFailure(squashAtomCommandFailure(result), "Try refreshing again."), + }); + return; + } refreshDetail(); setRefreshToken((token) => token + 1); } finally { From 927d965507f428786b6a8974513527a93793b730 Mon Sep 17 00:00:00 2001 From: Lars Nieuwenhuis <35393046+lnieuwenhuis@users.noreply.github.com> Date: Sat, 12 Sep 2026 20:36:48 +0200 Subject: [PATCH 8/8] fix(web): ignore superseded pull request refreshes --- .../usePullRequestRefresh.test.tsx | 44 +++++++++++++++++++ .../pullRequest/usePullRequestRefresh.ts | 26 ++++++++--- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/apps/web/src/components/pullRequest/usePullRequestRefresh.test.tsx b/apps/web/src/components/pullRequest/usePullRequestRefresh.test.tsx index 79cd6c0a7d11..75d35b4a3c40 100644 --- a/apps/web/src/components/pullRequest/usePullRequestRefresh.test.tsx +++ b/apps/web/src/components/pullRequest/usePullRequestRefresh.test.tsx @@ -232,6 +232,50 @@ describe("mounted pull request refresh sequencing", () => { expect(renderer!.root.findByType("button").props.disabled).toBe(false); }); + it.each(["success", "failure"])( + "ignores an older manual %s while a forced refresh is pending", + async (outcome) => { + await render(); + const older = invalidation(); + await act(async () => { + void renderer!.root.findByType("button").props.onClick(); + }); + const newer = invalidation(); + await render({ forcedRefreshToken: 1 }); + await (outcome === "success" ? older.succeed() : older.fail()); + expect(renderer!.root.findByType("button").props.disabled).toBe(true); + expect(refreshDetail).not.toHaveBeenCalled(); + expect(notify).not.toHaveBeenCalled(); + expect(diffRefreshes()).toBe("0"); + await newer.succeed(); + expect(renderer!.root.findByType("button").props.disabled).toBe(false); + expect(refreshDetail).toHaveBeenCalledOnce(); + expect(diffRefreshes()).toBe("1"); + }, + ); + + it.each(["scope", "unmount"])("ignores a manual refresh after %s changes", async (change) => { + await render(); + const pending = invalidation(); + await act(async () => { + void renderer!.root.findByType("button").props.onClick(); + }); + if (change === "scope") { + await render({ + scopeKey: `${props.scopeKey}:other`, + reference: { ...props.reference, number: 8 }, + }); + expect(renderer!.root.findByType("button").props.disabled).toBe(false); + } else { + await act(async () => renderer!.unmount()); + renderer = null; + } + await pending.succeed(); + expect(refreshDetail).not.toHaveBeenCalled(); + expect(notify).not.toHaveBeenCalled(); + if (renderer) expect(diffRefreshes()).toBe("0"); + }); + it("awaits full invalidation before a page refresh", async () => { await render(); const pending = invalidation(); diff --git a/apps/web/src/components/pullRequest/usePullRequestRefresh.ts b/apps/web/src/components/pullRequest/usePullRequestRefresh.ts index d35734d552df..7fa0862f8610 100644 --- a/apps/web/src/components/pullRequest/usePullRequestRefresh.ts +++ b/apps/web/src/components/pullRequest/usePullRequestRefresh.ts @@ -1,6 +1,6 @@ import { squashAtomCommandFailure } from "@t3tools/client-runtime/state/runtime"; import type { EnvironmentId, PullRequestDetail, PullRequestRef } from "@t3tools/contracts"; -import { useCallback, useEffect, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { useLiveRefresh } from "~/hooks/useLiveRefresh"; import { pullRequestEnvironment } from "~/state/pullRequests"; import { useAtomCommand } from "~/state/use-atom-command"; @@ -79,11 +79,26 @@ export function usePullRequestRefresh({ useLiveRefresh(() => void refreshDetailFromHost(), { key: `pull-request:${scopeKey}`, }); - const [isInvalidating, setIsInvalidating] = useState(false); + const refreshScope = useMemo(() => ({ key: scopeKey }), [scopeKey]); + const activeRefreshScope = useRef(null); + const refreshGeneration = useRef(0); + const [pendingScope, setPendingScope] = useState(null); + useEffect(() => { + activeRefreshScope.current = refreshScope; + return () => { + activeRefreshScope.current = null; + refreshGeneration.current += 1; + }; + }, [refreshScope]); + const isInvalidating = pendingScope === refreshScope; + const refreshFromHost = useCallback(async () => { - setIsInvalidating(true); + const generation = ++refreshGeneration.current; + setPendingScope(refreshScope); try { const result = await invalidate({ environmentId, input: { reference } }); + if (activeRefreshScope.current !== refreshScope || generation !== refreshGeneration.current) + return; if (result._tag === "Failure") { toastManager.add({ type: "error", @@ -95,9 +110,10 @@ export function usePullRequestRefresh({ refreshDetail(); setRefreshToken((token) => token + 1); } finally { - setIsInvalidating(false); + if (activeRefreshScope.current === refreshScope && generation === refreshGeneration.current) + setPendingScope(null); } - }, [environmentId, invalidate, reference, refreshDetail]); + }, [environmentId, invalidate, reference, refreshDetail, refreshScope]); // A refresh asked for by the page: the detail, and through the token below, the diff with it. const appliedForcedToken = useRef(forcedRefreshToken); useEffect(() => {