authorgravatar for maria@kuuro.netmaria <maria@kuuro.net> 2026-09-02 22:03:35-04:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-09-02 22:03:35-04:00
logd897641d738c67dd7c12cbb3a273b43fe17a5eb1
tree258618b72b87deb5388403a0b2872bf4c7e1bb1b
parent355fbd96d5e90b52961ac5f1e035112fc3bad4a9
signature Signed by PGP key B5690EEEBB952194

fix(pull-requests): keep cached PR chrome on reopen (#9294)

Co-authored-by: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>

5 files changed, 462 insertions(+), 22 deletions(-)

apps/server/src/pullRequest/PullRequestService.test.ts+170-2
......@@ -1,4 +1,5 @@
11import { assert, it } from "@effect/vitest";
2import * as Deferred from "effect/Deferred";
23import * as Effect from "effect/Effect";
34import * as Layer from "effect/Layer";
45import * as TestClock from "effect/testing/TestClock";
......@@ -77,6 +78,27 @@ function changeRequest(number: number, updatedAt: string): ProviderChangeRequest
7778 };
7879}
7980
81function hostedChangeRequest(body: string, additions = 1) {
82 return {
83 ...changeRequest(1, "2026-07-02T00:00:00Z"),
84 body,
85 additions,
86 changedFiles: 2,
87 mergedAt: null,
88 closedAt: null,
89 reviewers: [],
90 checks: [],
91 mergeCapabilities: { merge: true, squash: true, rebase: true },
92 viewerPermissions: {
93 actions: ["merge"] as const,
94 comment: true,
95 resolve: true,
96 verdicts: ["comment", "approve", "request-changes"] as const,
97 requestReviewers: true,
98 },
99 };
100}
101
80102function unusable(provider: SourceControlProviderKind, reason: "missing-tool" | "unauthenticated") {
81103 return new PullRequestProviderError({
82104 provider,
......@@ -2934,7 +2956,7 @@ it.effect(
29342956 }),
29352957);
29362958
2937it.effect("shares linked summaries and only recovers transient failures for display reads", () =>
2959it.effect("shares linked summaries and reuses them for display without asking the host again", () =>
29382960 Effect.gen(function* () {
29392961 let calls = 0;
29402962 let failing = false;
......@@ -2984,7 +3006,8 @@ it.effect("shares linked summaries and only recovers transient failures for disp
29843006
29853007 const stale = yield* service.summary(reference);
29863008 assert.strictEqual(stale.updatedAt, "2026-07-02T00:00:00Z");
2987 assert.strictEqual(calls, 3);
3009 // Display reads keep the last title and state rather than asking the host again.
3010 assert.strictEqual(calls, 2);
29883011
29893012 yield* service.invalidate({ reference });
29903013 const invalidated = yield* Effect.flip(service.summary(reference));
......@@ -2992,6 +3015,151 @@ it.effect("shares linked summaries and only recovers transient failures for disp
29923015 }),
29933016);
29943017
3018it.effect("answers a known pull request immediately while the host refreshes", () =>
3019 Effect.gen(function* () {
3020 const gate = yield* Deferred.make<void>();
3021 let calls = 0;
3022 const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 };
3023 const service = yield* makeService({
3024 projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })],
3025 providers: [
3026 fakeProvider("github", {
3027 getChangeRequest: () =>
3028 Effect.gen(function* () {
3029 calls += 1;
3030 if (calls > 1) yield* Deferred.await(gate);
3031 return hostedChangeRequest("cached body", 4);
3032 }),
3033 }),
3034 ],
3035 });
3036
3037 const first = yield* service.detail(reference);
3038 assert.strictEqual(first.body, "cached body");
3039 assert.strictEqual(first.additions, 4);
3040
3041 yield* TestClock.adjust("16 seconds");
3042 const second = yield* service.detail(reference);
3043 assert.strictEqual(second.body, "cached body");
3044 assert.strictEqual(second.additions, 4);
3045 yield* Effect.yieldNow;
3046 assert.strictEqual(calls, 2);
3047 }),
3048);
3049
3050it.effect("does not ask the host again for a linked summary it already holds", () =>
3051 Effect.gen(function* () {
3052 let calls = 0;
3053 const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 };
3054 const service = yield* makeService({
3055 projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })],
3056 providers: [
3057 fakeProvider("github", {
3058 getChangeRequestSummary: () =>
3059 Effect.sync(() => {
3060 calls += 1;
3061 return changeRequest(1, "2026-07-02T00:00:00Z");
3062 }),
3063 }),
3064 ],
3065 });
3066
3067 const first = yield* service.summary(reference);
3068 assert.strictEqual(first.title, "Change request 1");
3069 yield* TestClock.adjust("61 seconds");
3070 const second = yield* service.summary(reference);
3071 assert.strictEqual(second.title, "Change request 1");
3072 assert.strictEqual(calls, 1);
3073 }),
3074);
3075
3076it.effect("does not let a stale detail reopen overwrite a fresher linked summary", () =>
3077 Effect.gen(function* () {
3078 const gate = yield* Deferred.make<void>();
3079 let detailCalls = 0;
3080 let summaryTitle = "old title";
3081 let summaryState: "open" | "merged" = "open";
3082 const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 };
3083 const service = yield* makeService({
3084 projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })],
3085 providers: [
3086 fakeProvider("github", {
3087 getChangeRequest: () =>
3088 Effect.gen(function* () {
3089 detailCalls += 1;
3090 if (detailCalls > 1) yield* Deferred.await(gate);
3091 return hostedChangeRequest("old body", 4);
3092 }),
3093 getChangeRequestSummary: () =>
3094 Effect.succeed({
3095 ...changeRequest(1, "2026-07-02T00:00:00Z"),
3096 title: summaryTitle,
3097 state: summaryState,
3098 }),
3099 }),
3100 ],
3101 });
3102
3103 const first = yield* service.detail(reference);
3104 assert.strictEqual(first.title, "Change request 1");
3105
3106 summaryTitle = "merged title";
3107 summaryState = "merged";
3108 yield* TestClock.adjust("61 seconds");
3109 const settled = yield* service.summary(reference, { recoverTransientFailure: false });
3110 assert.strictEqual(settled.title, "merged title");
3111 assert.strictEqual(settled.state, "merged");
3112
3113 yield* TestClock.adjust("16 seconds");
3114 const stale = yield* service.detail(reference);
3115 assert.strictEqual(stale.title, "Change request 1");
3116 yield* Effect.yieldNow;
3117
3118 const display = yield* service.summary(reference);
3119 assert.strictEqual(display.title, "merged title");
3120 assert.strictEqual(display.state, "merged");
3121 assert.strictEqual(detailCalls, 2);
3122 }),
3123);
3124
3125it.effect("does not let a still-cached detail overwrite a fresher linked summary", () =>
3126 Effect.gen(function* () {
3127 let summaryTitle = "old title";
3128 let summaryState: "open" | "merged" = "open";
3129 const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 };
3130 const service = yield* makeService({
3131 projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })],
3132 providers: [
3133 fakeProvider("github", {
3134 getChangeRequest: () => Effect.succeed(hostedChangeRequest("old body", 4)),
3135 getChangeRequestSummary: () =>
3136 Effect.succeed({
3137 ...changeRequest(1, "2026-07-02T00:00:00Z"),
3138 title: summaryTitle,
3139 state: summaryState,
3140 }),
3141 }),
3142 ],
3143 });
3144
3145 const first = yield* service.detail(reference);
3146 assert.strictEqual(first.title, "Change request 1");
3147
3148 summaryTitle = "merged title";
3149 summaryState = "merged";
3150 const settled = yield* service.summary(reference, { recoverTransientFailure: false });
3151 assert.strictEqual(settled.state, "merged");
3152
3153 const cached = yield* service.detail(reference);
3154 assert.strictEqual(cached.title, "Change request 1");
3155 yield* Effect.yieldNow;
3156
3157 const display = yield* service.summary(reference);
3158 assert.strictEqual(display.title, "merged title");
3159 assert.strictEqual(display.state, "merged");
3160 }),
3161);
3162
29953163it.effect("keeps recent detail on a transient refresh failure but not after invalidation", () =>
29963164 Effect.gen(function* () {
29973165 let failing = false;
apps/server/src/pullRequest/PullRequestService.ts+54-3
......@@ -2001,7 +2001,25 @@ export const make = Effect.gen(function* () {
20012001 },
20022002 }),
20032003 );
2004 return { read, record };
2004 /**
2005 * A change request already read does not wait on the host again. `reuse` answers from
2006 * what we hold and spends nothing — title, author, and state barely move, and a linked
2007 * thread already names the change request. `revalidate` answers the same way and
2008 * refreshes behind it, so line counts and the rest can change in place.
2009 */
2010 const serveHeld = (
2011 key: string,
2012 effect: Effect.Effect<A, PullRequestError>,
2013 mode: "reuse" | "revalidate",
2014 ) => {
2015 const snapshot = held.get(key);
2016 if (snapshot === undefined) return read(key, effect);
2017 if (mode === "reuse") return Effect.succeed(snapshot.value);
2018 return Effect.sync(() => runFork(Effect.ignore(read(key, effect)))).pipe(
2019 Effect.as(snapshot.value),
2020 );
2021 };
2022 return { peek: (key: string) => held.get(key)?.value, read, record, serveHeld };
20052023 };
20062024 const lastGoodSummary = makeLastGoodRead<PullRequestSummary>(DETAIL_CACHE_CAPACITY);
20072025 const lastGoodDetail = makeLastGoodRead<PullRequestDetail>(DETAIL_CACHE_CAPACITY);
......@@ -2059,7 +2077,7 @@ export const make = Effect.gen(function* () {
20592077 const cached = Cache.get(summaryCache, key);
20602078 return options?.recoverTransientFailure === false
20612079 ? cached.pipe(Effect.tap((value) => lastGoodSummary.record(key, value)))
2062 : lastGoodSummary.read(key, cached);
2080 : lastGoodSummary.serveHeld(key, cached, "reuse");
20632081 };
20642082
20652083 // Keys serialize positionally and parse back in the lookup, so the cache is the only holder
......@@ -2149,9 +2167,42 @@ export const make = Effect.gen(function* () {
21492167 timeToLive: (exit) => (Exit.isSuccess(exit) ? DETAIL_CACHE_TTL : Duration.zero),
21502168 },
21512169 );
2170 const summaryFromDetail = (detail: PullRequestDetail): PullRequestSummary => ({
2171 provider: detail.provider,
2172 projectId: detail.projectId,
2173 repository: detail.repository,
2174 number: detail.number,
2175 title: detail.title,
2176 url: detail.url,
2177 state: detail.state,
2178 headBranch: detail.headBranch,
2179 baseBranch: detail.baseBranch,
2180 updatedAt: detail.updatedAt,
2181 });
2182 const shouldReplaceHeldSummary = (key: string, next: PullRequestSummary) => {
2183 const current = lastGoodSummary.peek(key);
2184 if (current === undefined) return true;
2185 if (current.state === "merged" && next.state !== "merged") return false;
2186 return next.updatedAt >= current.updatedAt;
2187 };
21522188 const detail: PullRequestService["Service"]["detail"] = (input) => {
21532189 const key = refCacheKey(input);
2154 return lastGoodDetail.read(key, Cache.get(detailCache, key));
2190 // Record the summary from a host or cache read, not the stale value
2191 // `serveHeld` returns immediately. Skip the write when that read is older
2192 // than a later strict summary — display reuse would otherwise keep the
2193 // regression and never ask the host again.
2194 return lastGoodDetail.serveHeld(
2195 key,
2196 Cache.get(detailCache, key).pipe(
2197 Effect.tap((value) => {
2198 const summary = summaryFromDetail(value);
2199 return shouldReplaceHeldSummary(key, summary)
2200 ? lastGoodSummary.record(key, summary)
2201 : Effect.void;
2202 }),
2203 ),
2204 "revalidate",
2205 );
21552206 };
21562207
21572208 const activityCache = yield* Cache.makeWith(
apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx+43-1
......@@ -120,10 +120,13 @@ import {
120120 pullRequestFindingKey,
121121 pullRequestHandoffLabels,
122122 readableFailure,
123 readPullRequestDetailSnapshot,
124 resolveDisplayedPullRequestDetail,
123125 resolvePullRequestPrimaryControl,
124126 resolveBaseFreshness,
125127 type PullRequestFinding,
126128 shouldRefreshPullRequestActivity,
129 writePullRequestDetailSnapshot,
127130} from "./pullRequestDetail.logic";
128131import { canEditPullRequestChangeRequest } from "./pullRequestEditing.logic";
129132import {
......@@ -558,7 +561,44 @@ export function PullRequestDetailPanel({
558561 const activityQuery = useEnvironmentQuery(
559562 pullRequestEnvironment.activity({ environmentId, input: reference }),
560563 );
561 const coreDetail = detailQuery.data;
564 const [cachedDetail, setCachedDetail] = useState(() =>
565 readPullRequestDetailSnapshot(
566 typeof window === "undefined" ? undefined : window.localStorage,
567 environmentId,
568 reference,
569 ),
570 );
571 useEffect(() => {
572 setCachedDetail(
573 readPullRequestDetailSnapshot(
574 typeof window === "undefined" ? undefined : window.localStorage,
575 environmentId,
576 reference,
577 ),
578 );
579 }, [environmentId, pullRequestKey, reference.projectId, reference.repository, reference.number]);
580 useEffect(() => {
581 if (detailQuery.data === null) return;
582 writePullRequestDetailSnapshot(
583 typeof window === "undefined" ? undefined : window.localStorage,
584 environmentId,
585 reference,
586 detailQuery.data,
587 );
588 setCachedDetail(detailQuery.data);
589 }, [
590 detailQuery.data,
591 environmentId,
592 pullRequestKey,
593 reference.projectId,
594 reference.repository,
595 reference.number,
596 ]);
597 const coreDetail = resolveDisplayedPullRequestDetail({
598 live: detailQuery.data,
599 cached: cachedDetail,
600 reference,
601 });
562602 const activity = activityQuery.data;
563603 const detail = useMemo(
564604 () =>
......@@ -1240,6 +1280,8 @@ export function PullRequestDetailPanel({
12401280 ).length
12411281 : 0;
12421282
1283 // A reopen already has last time's title, author, and counts. Keep them on screen
1284 // and let the live read replace fields — especially the diff counts — in place.
12431285 if (detailQuery.isPending && !detail) {
12441286 return <PullRequestDetailGhost />;
12451287 }
apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts+104
......@@ -2,6 +2,7 @@ import {
22 PullRequestAction,
33 type PullRequestCheck,
44 type PullRequestComment,
5 type PullRequestDetail,
56 type PullRequestDetailView,
67 type PullRequestReviewThread,
78} from "@t3tools/contracts";
......@@ -31,12 +32,15 @@ import {
3132 pullRequestHandoffLabels,
3233 pullRequestReviewOutcome,
3334 readableFailure,
35 readPullRequestDetailSnapshot,
36 resolveDisplayedPullRequestDetail,
3437 resolvePullRequestPrimaryControl,
3538 shouldRefreshPullRequestActivity,
3639 resolveBaseFreshness,
3740 buildPullRequestTimeline,
3841 describePullRequestState,
3942 editPullRequestThreadComment,
43 writePullRequestDetailSnapshot,
4044} from "./pullRequestDetail.logic";
4145import type { ReviewCommentContext } from "~/reviewCommentContext";
4246
......@@ -1317,3 +1321,103 @@ describe("which actions need the host read again after they run", () => {
13171321 }
13181322 });
13191323});
1324
1325describe("cached pull request detail", () => {
1326 const reference = { projectId: "project-1", repository: "acme/web", number: 7 };
1327 const detail = (overrides: Partial<PullRequestDetail> = {}): PullRequestDetail =>
1328 ({
1329 provider: "github",
1330 capabilities: {
1331 diff: true,
1332 comment: true,
1333 actions: ["merge"],
1334 mergeMethods: ["merge"],
1335 search: true,
1336 review: {
1337 inlineComment: true,
1338 reply: true,
1339 resolve: true,
1340 verdicts: ["comment", "approve", "request-changes"],
1341 },
1342 reviewers: { request: true, listCandidates: true },
1343 },
1344 viewerPermissions: {
1345 actions: ["merge"],
1346 comment: true,
1347 resolve: true,
1348 verdicts: ["comment", "approve", "request-changes"],
1349 requestReviewers: true,
1350 },
1351 projectId: "project-1",
1352 projectTitle: "web",
1353 workspaceRoot: "/repo",
1354 repository: "acme/web",
1355 number: 7,
1356 title: "Cache the title",
1357 body: "who made it",
1358 url: "https://github.com/acme/web/pull/7",
1359 author: { login: "octocat", name: null, avatarUrl: "https://avatars.example/octocat" },
1360 state: "open",
1361 isDraft: false,
1362 mergeability: "mergeable",
1363 additions: 12,
1364 deletions: 3,
1365 changedFiles: 2,
1366 headBranch: "feat/cache",
1367 baseBranch: "main",
1368 createdAt: "2026-07-01T00:00:00.000Z",
1369 updatedAt: "2026-07-02T00:00:00.000Z",
1370 mergedAt: null,
1371 closedAt: null,
1372 reviewers: [],
1373 labels: [],
1374 checks: [],
1375 mergeCapabilities: { merge: true, squash: true, rebase: true },
1376 ...overrides,
1377 }) as PullRequestDetail;
1378
1379 const makeStorage = () => {
1380 const held = new Map<string, string>();
1381 return {
1382 getItem: (key: string) => held.get(key) ?? null,
1383 setItem: (key: string, value: string) => void held.set(key, value),
1384 };
1385 };
1386
1387 it("hydrates the last title, author, and counts so a reopen does not ghost the tab", () => {
1388 const storage = makeStorage();
1389 writePullRequestDetailSnapshot(storage, "env-1", reference, detail());
1390 const snapshot = readPullRequestDetailSnapshot(storage, "env-1", reference);
1391 expect(snapshot?.title).toBe("Cache the title");
1392 expect(snapshot?.author?.login).toBe("octocat");
1393 expect(snapshot?.additions).toBe(12);
1394 expect(snapshot?.deletions).toBe(3);
1395 });
1396
1397 it("keeps a cached tab painted while the live read replaces the counts", () => {
1398 const cached = detail();
1399 const live = detail({ additions: 40, deletions: 9, title: "Cache the title" });
1400 expect(resolveDisplayedPullRequestDetail({ live, cached, reference })?.additions).toBe(40);
1401 expect(resolveDisplayedPullRequestDetail({ live: null, cached, reference })?.additions).toBe(
1402 12,
1403 );
1404 });
1405
1406 it("does not paint another change request's snapshot", () => {
1407 expect(
1408 resolveDisplayedPullRequestDetail({
1409 live: null,
1410 cached: detail({ number: 8 }),
1411 reference,
1412 }),
1413 ).toBeNull();
1414 expect(readPullRequestDetailSnapshot(makeStorage(), "env-2", reference)).toBeNull();
1415 });
1416
1417 it("shrugs off corrupt storage and no storage at all", () => {
1418 const storage = makeStorage();
1419 storage.setItem("t3.pullRequests.detail:env-1:project-1:acme/web#7", "{not json");
1420 expect(readPullRequestDetailSnapshot(storage, "env-1", reference)).toBeNull();
1421 expect(readPullRequestDetailSnapshot(undefined, "env-1", reference)).toBeNull();
1422 });
1423});
apps/web/src/components/pullRequest/pullRequestDetail.logic.ts+91-16
......@@ -1,19 +1,22 @@
1import type {
2 PullRequestAction,
3 PullRequestActor,
4 PullRequestBaseComparison,
5 PullRequestCheck,
6 PullRequestChecksState,
7 PullRequestComment,
8 PullRequestCommit,
9 PullRequestDetailView,
10 PullRequestMergeability,
11 PullRequestReaction,
12 PullRequestReviewThread,
13 PullRequestState,
14 PullRequestUpdateMethod,
15 SourceControlProviderKind,
16 VcsRef,
1import * as Schema from "effect/Schema";
2
3import {
4 PullRequestDetail,
5 type PullRequestAction,
6 type PullRequestActor,
7 type PullRequestBaseComparison,
8 type PullRequestCheck,
9 type PullRequestChecksState,
10 type PullRequestComment,
11 type PullRequestCommit,
12 type PullRequestDetailView,
13 type PullRequestMergeability,
14 type PullRequestReaction,
15 type PullRequestReviewThread,
16 type PullRequestState,
17 type PullRequestUpdateMethod,
18 type SourceControlProviderKind,
19 type VcsRef,
1720} from "@t3tools/contracts";
1821
1922import { inferReviewCommentFenceLanguage, type ReviewCommentContext } from "~/reviewCommentContext";
......@@ -1009,3 +1012,75 @@ const ACTION_NEEDS_HOST_REFRESH: Record<PullRequestAction, boolean> = {
10091012export function pullRequestActionNeedsHostRefresh(action: PullRequestAction): boolean {
10101013 return ACTION_NEEDS_HOST_REFRESH[action];
10111014}
1015
1016type SnapshotStorage = Pick<Storage, "getItem" | "setItem">;
1017
1018export interface PullRequestDetailSnapshotRef {
1019 readonly projectId: string;
1020 readonly repository: string;
1021 readonly number: number;
1022}
1023
1024const pullRequestDetailSnapshotKey = (
1025 environmentId: string,
1026 reference: PullRequestDetailSnapshotRef,
1027) =>
1028 `t3.pullRequests.detail:${environmentId}:${reference.projectId}:${reference.repository}#${reference.number}`;
1029
1030const decodeDetailSnapshot = Schema.decodeUnknownOption(PullRequestDetail);
1031
1032/**
1033 * The last detail answered for this change request, brought back across a reload. The registry
1034 * the queries live in is recreated with the renderer, so without this a reopen cold-starts
1035 * into a full-tab ghost even though the title, author, and the rest barely moved. Hydrated,
1036 * the chrome stays and the live read replaces fields in place — line counts included.
1037 */
1038export function readPullRequestDetailSnapshot(
1039 storage: SnapshotStorage | undefined,
1040 environmentId: string,
1041 reference: PullRequestDetailSnapshotRef,
1042): PullRequestDetail | null {
1043 try {
1044 const raw = storage?.getItem(pullRequestDetailSnapshotKey(environmentId, reference));
1045 if (!raw) return null;
1046 const decoded = decodeDetailSnapshot(JSON.parse(raw));
1047 return decoded._tag === "Some" ? decoded.value : null;
1048 } catch {
1049 return null;
1050 }
1051}
1052
1053export function writePullRequestDetailSnapshot(
1054 storage: SnapshotStorage | undefined,
1055 environmentId: string,
1056 reference: PullRequestDetailSnapshotRef,
1057 detail: PullRequestDetail,
1058): void {
1059 try {
1060 storage?.setItem(
1061 pullRequestDetailSnapshotKey(environmentId, reference),
1062 JSON.stringify(detail),
1063 );
1064 } catch {
1065 // Quota or a private-mode store: the next open waits on the live read, which is the
1066 // cold start this snapshot exists to avoid, not a failure of its own.
1067 }
1068}
1069
1070/** Live host state wins; a snapshot is only the same change request, never a neighbour's. */
1071export function resolveDisplayedPullRequestDetail(input: {
1072 readonly live: PullRequestDetail | null;
1073 readonly cached: PullRequestDetail | null;
1074 readonly reference: PullRequestDetailSnapshotRef;
1075}): PullRequestDetail | null {
1076 if (input.live !== null) return input.live;
1077 if (
1078 input.cached !== null &&
1079 input.cached.projectId === input.reference.projectId &&
1080 input.cached.repository.toLowerCase() === input.reference.repository.toLowerCase() &&
1081 input.cached.number === input.reference.number
1082 ) {
1083 return input.cached;
1084 }
1085 return null;
1086}