fix(web): pull request panel polls go around the server hold

apps/server/src/pullRequest/PullRequestService.test.ts

@@ -3351,6 +3351,193 @@ 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<void>();
+    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("a detail-scoped invalidate refreshes detail without stranding the held diff", () =>
+  Effect.gen(function* () {
+    const diffGate = yield* Deferred.make<void>();
+    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("a changed revision invalidates every held diff page before reloading", () =>
+  Effect.gen(function* () {
+    const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 };
+    const calls: Array<string | undefined> = [];
+    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);
+              return {
+                patch: `${revision}:${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",
+    );
+
+    revision = "new";
+    yield* service.invalidate({ reference, scope: "detail" });
+    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"]);
+
+    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(
+      (yield* service.diff({ ...reference, cursor: "page-2" })).patch,
+      "new:page-2",
+    );
+    assert.deepStrictEqual(calls, [undefined, "page-2", undefined, "page-2"]);
+  }),
+);
+
 it.effect("does not ask the host again for a linked summary it already holds", () =>
   Effect.gen(function* () {
     let calls = 0;

apps/server/src/pullRequest/PullRequestService.ts

@@ -2128,14 +2128,20 @@ 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<string, number>();
+  const diffEpochs = new Map<string, number>();
   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]);
   // Counts belong to a PR, not a filtered page. Background reads and filter changes reuse
@@ -2153,13 +2159,18 @@ export const make = Effect.gen(function* () {
       if (oldest !== undefined) recentStats.delete(oldest);
     }
   };
-  const bumpRefEpoch = (ref: PullRequestRef) => {
+  const bumpMapEpoch = (epochs: Map<string, number>, 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. */
@@ -2390,19 +2401,28 @@ export const make = Effect.gen(function* () {
       },
     },
   );
+  const lastGoodDiffRevision = makeLastGoodRead<string>(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([
-      refEpoch(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(
@@ -2465,7 +2485,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;

apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx

@@ -698,32 +698,51 @@ export function PullRequestDetailPanel({
   }, [activityQuery.refresh, detailQuery.refresh]);
   const [refreshToken, setRefreshToken] = useState(0);
   const codeRefreshToken = refreshToken + (turnRefresh ?? 0);
+  const invalidate = useAtomCommand(pullRequestEnvironment.invalidate, { reportFailure: false });
   const activityRevision = useRef<{ readonly key: string; readonly updatedAt: string } | null>(
     null,
   );
   useEffect(() => {
-    if (!coreDetail) return;
+    if (!coreDetail) {
+      activityRevision.current = null;
+      return;
+    }
     const next = { key: tabScopeKey, updatedAt: coreDetail.updatedAt };
-    if (shouldRefreshPullRequestActivity(activityRevision.current, next)) {
+    if (
+      activityRevision.current?.key === next.key &&
+      activityRevision.current.updatedAt === next.updatedAt
+    )
+      return;
+    const previous = activityRevision.current;
+    const changed = shouldRefreshPullRequestActivity(previous, next);
+    activityRevision.current = next;
+    if (!changed) return;
+    // A changed revision must miss the held diff before the Code tab reads its first page.
+    // Later revisions or another PR supersede this refresh while invalidation is in flight.
+    void invalidate({ environmentId, input: { reference } }).then((result) => {
+      if (activityRevision.current !== next) return;
+      if (result._tag === "Failure") {
+        activityRevision.current = previous;
+        toastManager.add({
+          type: "error",
+          title: "The pull request could not be refreshed",
+          description: readableFailure(squashAtomCommandFailure(result), "Try refreshing again."),
+        });
+        return;
+      }
       activityQuery.refresh();
       setRefreshToken((token) => token + 1);
-    }
-    activityRevision.current = next;
-  }, [activityQuery.refresh, coreDetail, tabScopeKey]);
-  // Reuse activity and diff until core detail reports a changed revision. 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();
-    },
-    { key: `pull-request:${environmentId}:${pullRequestKey}` },
-  );
-  // 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 });
+    });
+  }, [activityQuery.refresh, coreDetail, environmentId, invalidate, reference, tabScopeKey]);
+  // Poll fresh metadata without invalidating cached diff pages. A changed detail revision
+  // refreshes activity and the Code tab above; unchanged polls preserve loaded slices.
+  const refreshDetailFromHost = useCallback(async () => {
+    await invalidate({ environmentId, input: { reference, scope: "detail" } });
+    detailQuery.refresh();
+  }, [detailQuery.refresh, environmentId, invalidate, reference]);
+  useLiveRefresh(() => void refreshDetailFromHost(), {
+    key: `pull-request:${environmentId}:${pullRequestKey}`,
+  });
   const [isInvalidating, setIsInvalidating] = useState(false);
   const refreshFromHost = useCallback(async () => {
     setIsInvalidating(true);

packages/contracts/src/pullRequest.ts

@@ -687,9 +687,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;