perf(pull-requests): faster pull request timeline, code tab, summary and search

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

@@ -2663,6 +2663,9 @@ it.effect("isolates routed caches for two credentials belonging to the same acco
         host: "github.com",
         expectedAccountId: "101",
       };
+      if (operation === "checks") {
+        yield* service.withRoutingCredential(reference, service.detail(reference));
+      }
       yield* service.withRoutingCredential(reference, readOperation(reference));
       credential = "restricted";
       for (const allowStale of [false, true]) {
@@ -2838,6 +2841,48 @@ it.effect("deduplicates routed PR reports without dropping draft changes", () =>
   }),
 );
 
+it.effect(
+  "reuses fresh detail checks without extending their refresh interval on detail cache hits",
+  () =>
+    Effect.gen(function* () {
+      let reads = 0;
+      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: [
+          fakePullRequestProvider(SourceControlProviderKind.make("github"), {
+            getChangeRequest: () => Effect.succeed(hostedChangeRequest("Description")),
+            getChangeRequestChecks: () =>
+              Effect.sync(() => {
+                reads += 1;
+                return { state: "closed" as const, checks: [] };
+              }),
+          }),
+        ],
+      });
+      yield* TestClock.setTime(1_000);
+      const detail = yield* service.detail(reference);
+      expect(yield* service.checks(reference)).toEqual({
+        state: detail.state,
+        checks: detail.checks,
+        observedAt: detail.observedAt,
+      });
+      expect(reads).toBe(0);
+      yield* TestClock.adjust("16 seconds");
+      yield* service.detail(reference);
+      expect(yield* service.checks(reference)).toMatchObject({
+        state: "closed",
+        observedAt: 17_000,
+      });
+      expect(reads).toBe(1);
+      yield* service.detail(reference);
+      expect((yield* service.checks(reference))?.state).toBe("closed");
+      expect(reads).toBe(1);
+    }),
+);
+
 it.effect("preserves check observation time on cache hits and advances it on refresh", () =>
   Effect.gen(function* () {
     let reads = 0;
@@ -4260,7 +4305,7 @@ it.effect.each(["commit", "credential"] as const)(
     }),
 );
 
-it.effect.each(["expiry", "refresh"] as const)(
+it.effect.each(["expiry", "commit expiry", "cache hit", "commit cache hit", "refresh"] as const)(
   "keeps a loaded diff's original membership scope through an in-flight %s",
   (boundary) =>
     Effect.gen(function* () {
@@ -4284,17 +4329,40 @@ it.effect.each(["expiry", "refresh"] as const)(
                   nextCursor: null,
                 };
               }),
-            getDiffFileContents: () => Effect.die("an expired file must not be read"),
+            getDiffFileContents: () => Effect.succeed({ oldContents: "old", newContents: "new" }),
           }),
         ],
       });
-      const ref = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 };
+      const ref = {
+        projectId: "p1" as ProjectId,
+        repository: "acme/web",
+        number: 1,
+        ...(boundary.includes("commit") ? { commit: "a".repeat(40) } : {}),
+      };
       const loading = yield* Effect.forkChild(service.diff(ref));
       yield* Deferred.await(started);
       if (boundary === "expiry") yield* TestClock.adjust("61 seconds");
+      else if (boundary === "commit expiry") yield* TestClock.adjust("11 minutes");
+      else if (boundary.includes("cache hit")) yield* TestClock.adjust("30 seconds");
       else yield* service.invalidate({ reference: ref });
       yield* Deferred.succeed(release, undefined);
       yield* Fiber.join(loading);
+      if (boundary.includes("cache hit")) {
+        yield* service.diff(ref);
+        yield* service.diff(ref);
+        yield* TestClock.adjust(boundary.includes("commit") ? "9 minutes" : "29 seconds");
+        assert.deepStrictEqual(
+          yield* service.diffFileContents({
+            ...ref,
+            changeType: "change",
+            oldPath: "private.ts",
+            newPath: "private.ts",
+          }),
+          { oldContents: "old", newContents: "new" },
+        );
+        assert.strictEqual(diffReads, 1);
+        yield* TestClock.adjust(boundary.includes("commit") ? "1 minute" : "2 seconds");
+      }
       const error = yield* service
         .diffFileContents({
           ...ref,

apps/server/src/pullRequest/PullRequestService.ts

@@ -141,9 +141,23 @@ const LIST_CACHE_CAPACITY = 64;
 const LIST_STATS_CACHE_CAPACITY = 32;
 const DETAIL_CACHE_CAPACITY = 128;
 const DIFF_CACHE_CAPACITY = 128;
-const MAX_CACHED_DIFF_PATCH_BYTES = 512 * 1024;
+// Each holder stays within this UTF-16 patch budget as well as its entry-count bound.
+const DIFF_CACHE_PATCH_BUDGET_BYTES = 32 * 1024 * 1024;
 const canCacheDiff = (value: PullRequestDiffResult) =>
-  value.patch.length * 2 <= MAX_CACHED_DIFF_PATCH_BYTES;
+  value.patch.length * 2 <= DIFF_CACHE_PATCH_BUDGET_BYTES;
+const diffCacheEvictions = (entries: Iterable<readonly [string, PullRequestDiffResult]>) => {
+  let bytes = 0;
+  const evictions: string[] = [];
+  for (const [key, value] of Array.from(entries).toReversed()) {
+    const size = value.patch.length * 2;
+    if (bytes + size > DIFF_CACHE_PATCH_BUDGET_BYTES) {
+      evictions.push(key);
+    } else {
+      bytes += size;
+    }
+  }
+  return evictions;
+};
 const FILES_VIEWED_CACHE_CAPACITY = 128;
 const VIEWER_CACHE_CAPACITY = 32;
 
@@ -681,6 +695,7 @@ export const make = Effect.gen(function* () {
   const rateLimits = yield* SourceControlRateLimit.SourceControlRateLimit;
   const filesViewedStore = yield* PullRequestFilesViewed.PullRequestFilesViewedRepository;
   const readCache = yield* PullRequestReadCache.PullRequestReadCache;
+  const clock = yield* Clock.Clock;
 
   /**
    * Where each project's remote is served from, by the remote's recognised base URL: a remote no
@@ -2617,6 +2632,11 @@ export const make = Effect.gen(function* () {
           if (oldest !== undefined) held.delete(oldest);
         }
         held.set(key, { at, value });
+        for (const evicted of diffCacheEvictions(
+          Array.from(held, ([key, snapshot]) => [key, snapshot.value] as const),
+        )) {
+          held.delete(evicted);
+        }
         armDiffSweep();
       });
     const serve = <E>(key: string, read: Effect.Effect<PullRequestDiffResult, E>) => {
@@ -2964,7 +2984,20 @@ export const make = Effect.gen(function* () {
     },
     {
       capacity: DETAIL_CACHE_CAPACITY,
-      timeToLive: (exit) => (Exit.isSuccess(exit) ? CHECKS_CACHE_TTL : Duration.zero),
+      // Expire from the read's start so checks seeded from a slow detail read keep their age.
+      timeToLive: (exit) =>
+        !Exit.isSuccess(exit)
+          ? Duration.zero
+          : exit.value === null
+            ? CHECKS_CACHE_TTL
+            : Duration.millis(
+                Math.max(
+                  0,
+                  exit.value.observedAt +
+                    Duration.toMillis(CHECKS_CACHE_TTL) -
+                    clock.currentTimeMillisUnsafe(),
+                ),
+              ),
     },
   );
 
@@ -2998,6 +3031,18 @@ export const make = Effect.gen(function* () {
       return detailUncached(refOfCacheKey(key)).pipe(
         Effect.tap(
           Effect.fn("PullRequestService.recordDetailStats")(function* (value: PullRequestDetail) {
+            // GitHub detail includes the same checks; the checks TTL counts from their observation.
+            if (
+              value.provider === "github" &&
+              value.observedAt !== undefined &&
+              !(yield* Cache.has(checksCache, key))
+            ) {
+              yield* Cache.set(checksCache, key, {
+                state: value.state,
+                checks: value.checks,
+                observedAt: value.observedAt,
+              });
+            }
             recordStats(
               statsKey,
               {
@@ -3208,10 +3253,10 @@ export const make = Effect.gen(function* () {
         ...(commit === null ? {} : { commit }),
       } as PullRequestDiffInput).pipe(
         observeRead,
-        Effect.map(({ value, observedAt }) => {
+        Effect.tap(({ value, observedAt }) => {
           // Keep the loader's epoch and read time if a refresh occurs during the provider read.
           diffFiles.record(JSON.stringify([reference, cursor, commit]), value, observedAt);
-          return value;
+          return Effect.void;
         }),
       );
     },
@@ -3220,7 +3265,10 @@ export const make = Effect.gen(function* () {
       timeToLive: (exit, key) => {
         if (!Exit.isSuccess(exit)) return Duration.zero;
         const commit = (JSON.parse(key) as ReadonlyArray<unknown>)[2];
-        return commit === null ? DIFF_CACHE_TTL : COMMIT_DIFF_CACHE_TTL;
+        const ttl = Duration.toMillis(commit === null ? DIFF_CACHE_TTL : COMMIT_DIFF_CACHE_TTL);
+        return Duration.millis(
+          Math.max(0, exit.value.observedAt + ttl - clock.currentTimeMillisUnsafe()),
+        );
       },
     },
   );
@@ -3246,7 +3294,7 @@ export const make = Effect.gen(function* () {
       if (held > 0) armDiffSweep();
     }),
   );
-  const diff: PullRequestService["Service"]["diff"] = (input) => {
+  const readDiff = (input: PullRequestDiffInput) => {
     const key = JSON.stringify([
       refCacheKey(input),
       input.cursor ?? null,
@@ -3255,22 +3303,24 @@ export const make = Effect.gen(function* () {
         ? (lastGoodSummary.peek(refCacheKey(input))?.updatedAt ?? null)
         : null,
     ]);
-    const read = Cache.get(diffCache, key).pipe(
-      Effect.tap((value) =>
-        canCacheDiff(value)
-          ? Effect.void
-          : Cache.getSuccess(diffCache, key).pipe(
-              Effect.flatMap((current) =>
-                Option.isSome(current) && current.value === value
-                  ? Cache.invalidate(diffCache, key)
-                  : Effect.void,
-              ),
-              Effect.uninterruptible,
-            ),
+    const evictDiffs = Cache.entries(diffCache).pipe(
+      Effect.flatMap((entries) =>
+        Effect.forEach(
+          diffCacheEvictions(Array.from(entries, ([key, read]) => [key, read.value] as const)),
+          (evicted) => Cache.invalidate(diffCache, evicted),
+        ),
       ),
+      Effect.uninterruptible,
+    );
+    const read = Cache.get(diffCache, key).pipe(
+      Effect.ensuring(evictDiffs),
       Effect.ensuring(Effect.sync(armDiffSweep)),
     );
-    return staleDiff.serve(key, read);
+    return { key, read };
+  };
+  const diff: PullRequestService["Service"]["diff"] = (input) => {
+    const { key, read } = readDiff(input);
+    return staleDiff.serve(key, read.pipe(Effect.map((read) => read.value)));
   };
 
   const filesViewedCache = yield* Cache.makeWith(
@@ -3524,8 +3574,8 @@ export const make = Effect.gen(function* () {
           .pipe(Effect.mapError(toPullRequestError("diffFileContents")));
         return recordPaths(key, slice, now);
       }
-      const slice = yield* diff(input);
-      return record(key, slice, now);
+      const { value, observedAt } = yield* readDiff(input).read;
+      return record(key, value, observedAt);
     });
     return { read: credentialCached(read), record, fileKey };
   })();

apps/web/src/components/DiffWorkerPoolProvider.tsx

@@ -125,9 +125,7 @@ function DiffWorkerReady({ children }: { children?: ReactNode }) {
   );
 }
 
-export function DiffWorkerPoolProvider({ children }: { children?: ReactNode }) {
-  const resolvedTheme = useRenderedAppearance();
-  const diffThemeName = resolveDiffThemeName(resolvedTheme);
+export function useDiffWorkerPool(diffThemeName: DiffThemeName) {
   const [workerPoolSize] = useState(resolveDiffWorkerPoolSize);
   const subscribe = useStableCallback((onStoreChange: () => void) => {
     if (typeof window === "undefined") return () => {};
@@ -143,11 +141,17 @@ export function DiffWorkerPoolProvider({ children }: { children?: ReactNode }) {
       }, DIFF_WORKER_IDLE_TTL_MS);
     };
   });
-  const workerPool = useSyncExternalStore(
+  return useSyncExternalStore(
     subscribe,
     () => sharedWorkerPool?.pool,
     () => undefined,
   );
+}
+
+export function DiffWorkerPoolProvider({ children }: { children?: ReactNode }) {
+  const resolvedTheme = useRenderedAppearance();
+  const diffThemeName = resolveDiffThemeName(resolvedTheme);
+  const workerPool = useDiffWorkerPool(diffThemeName);
 
   return (
     <WorkerPoolContext value={workerPool}>

apps/web/src/components/RightPanelTabs.tsx

@@ -726,6 +726,7 @@ function PullRequestSurfaceIcon({
               : {}),
             repository: surface.repository,
             number: surface.number,
+            allowStale: false,
           },
         }),
   ).data;

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

@@ -8,6 +8,7 @@ import type {
   EnvironmentId,
   PullRequestDetailView,
   PullRequestDiffSide,
+  PullRequestDiffResult,
   PullRequestOmittedFileStat,
   PullRequestRef,
   PullRequestReviewPosition,
@@ -74,6 +75,7 @@ import { useEnvironmentQuery } from "~/state/query";
 import { useAtomCommand } from "~/state/use-atom-command";
 
 import { DiffPanelLoadingState } from "../DiffPanelShell";
+import { useDiffWorkerPool } from "../DiffWorkerPoolProvider";
 import { DiffCommentAnnotation } from "../diffs/DiffCommentAnnotation";
 import { DiffFileTree } from "../diffs/DiffFileTree";
 import { useCodeViewFileReveal } from "../diffs/useCodeViewFileReveal";
@@ -132,6 +134,7 @@ const COMMIT_PAGE_SIZE = 10;
 const PULL_REQUEST_FILE_TREE_STORAGE_KEY = "t3code.pullRequestFileTreeOpen";
 
 interface DiffSlice {
+  readonly source: PullRequestDiffResult;
   readonly cursor: string | null;
   readonly patch: string;
   readonly truncated: boolean;
@@ -148,13 +151,15 @@ const REPLACE_FILE_COUNTS_CSS = `
 
 const NO_SLICES: ReadonlyArray<DiffSlice> = [];
 
-const sliceParses = new WeakMap<DiffSlice, Map<string, RenderablePatch | null>>();
+// Query results outlive the panel's slices, so their parses survive closing and reopening
+// without retaining anything beyond the query cache's lifetime.
+const sliceParses = new WeakMap<PullRequestDiffResult, Map<string, RenderablePatch | null>>();
 
 function parseSlice(slice: DiffSlice, cacheKey: string, ignoreWhitespace: boolean) {
-  let parses = sliceParses.get(slice);
+  let parses = sliceParses.get(slice.source);
   if (parses === undefined) {
     parses = new Map();
-    sliceParses.set(slice, parses);
+    sliceParses.set(slice.source, parses);
   }
   if (parses.has(cacheKey)) return parses.get(cacheKey) ?? null;
   const parsed = getRenderablePatch(slice.patch, cacheKey, {
@@ -272,6 +277,8 @@ function PullRequestCodeTab({
   revealRequest?: { readonly path: string; readonly nonce: number } | null;
 }) {
   const resolvedTheme = useRenderedAppearance();
+  // Initialize the shared tokenizer and workers while the diff request is still pending.
+  useDiffWorkerPool(resolveDiffThemeName(resolvedTheme));
   const settings = useClientSettings((settings) => ({
     diffFilesCollapsed: settings.diffFilesCollapsed,
     diffIgnoreWhitespace: settings.diffIgnoreWhitespace,
@@ -353,6 +360,7 @@ function PullRequestCodeTab({
     setSliceState((previous) => {
       const slices = previous.key === scopeKey ? previous.slices : NO_SLICES;
       const next = {
+        source: data,
         cursor,
         patch: data.patch,
         truncated: data.truncated,

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

@@ -17,9 +17,9 @@ export function PullRequestCommentBody({
   useEffect(() => {
     const element = content.current;
     if (!element) return;
-    const measure = () => setOverflowing(element.getBoundingClientRect().height > 240);
-    measure();
-    return observeResize(element, measure);
+    return observeResize(element, ([entry]) => {
+      if (entry) setOverflowing(entry.contentRect.height > 240);
+    });
   }, []);
 
   return (