authorgravatar for maria@kuuro.netmaria <maria@kuuro.net> 2026-09-04 21:10:25-04:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-09-04 21:10:25-04:00
log110bbe6b55d684627a2481aebfd7c1ae3396011c
tree557efe56a4484ca1213bb6c0b861e3c0770bbad0
parentb5fb3fba0fb3dbd1bc2e29886232321cc06863d5
signature Signed by PGP key B5690EEEBB952194

fix(prs): reuse GitHub data and defer optional reads (#9835)

11 files changed, 420 insertions(+), 186 deletions(-)

apps/server/src/pullRequest/PullRequestService.test.ts+123
...@@ -3057,6 +3057,114 @@ it.effect("fills in the line counts for the rows it is given", () =>...@@ -3057,6 +3057,114 @@ it.effect("fills in the line counts for the rows it is given", () =>
3057 ]);3057 ]);
3058 }),3058 }),
3059);3059);
3060it.effect(
3061 "reuses counts across overlapping pages until expiry, explicit invalidation, or a reference changes",
3062 () =>
3063 Effect.gen(function* () {
3064 const asked: number[][] = [];
3065 const ref = (number: number) => ({
3066 projectId: "p1" as ProjectId,
3067 repository: "acme/web",
3068 number,
3069 });
3070 const service = yield* makeService({
3071 projects: [
3072 project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" }),
3073 ],
3074 providers: [
3075 fakeProvider("github", {
3076 listChangeRequestStats: (input) => {
3077 asked.push(input.changeRequests.map((ref) => ref.number));
3078 return Effect.succeed(
3079 input.changeRequests.map((ref) => ({ ...ref, additions: 12, deletions: 3 })),
3080 );
3081 },
3082 }),
3083 ],
3084 });
3085
3086 yield* service.listStats({ refs: [ref(1), ref(2)] });
3087 const overlapping = yield* service.listStats({ refs: [ref(2), ref(3)] });
3088 assert.deepStrictEqual(
3089 overlapping.stats.map((stat) => stat.number),
3090 [2, 3],
3091 );
3092 yield* service.listStats({ refs: [ref(1), ref(2), ref(3)] });
3093 assert.deepStrictEqual(asked, [[1, 2], [3]]);
3094
3095 yield* service.invalidate({ reference: ref(2) });
3096 yield* service.listStats({ refs: [ref(1), ref(2), ref(3)] });
3097 assert.deepStrictEqual(asked, [[1, 2], [3], [2]]);
3098
3099 yield* TestClock.adjust("61 seconds");
3100 yield* service.listStats({ refs: [ref(1), ref(2), ref(3)] });
3101 assert.deepStrictEqual(asked, [[1, 2], [3], [2], [1, 2, 3]]);
3102
3103 yield* service.refreshAfterTurn;
3104 yield* service.listStats({ refs: [ref(1)] });
3105 assert.deepStrictEqual(asked, [[1, 2], [3], [2], [1, 2, 3], [1]]);
3106
3107 yield* service.invalidate({});
3108 yield* service.listStats({ refs: [ref(1)] });
3109 assert.deepStrictEqual(asked, [[1, 2], [3], [2], [1, 2, 3], [1], [1]]);
3110 }),
3111);
3112
3113it.effect("reads the fresh diff when detail or summary discovers a changed revision", () =>
3114 Effect.gen(function* () {
3115 const summaryStarted = yield* Deferred.make<void>();
3116 const releaseSummary = yield* Deferred.make<void>();
3117 let revision = "2026-07-02T00:00:00Z";
3118 let patch = "old patch";
3119 let diffCalls = 0;
3120 const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 };
3121 const service = yield* makeService({
3122 projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })],
3123 providers: [
3124 fakeProvider("github", {
3125 getChangeRequest: () =>
3126 Effect.sync(() => ({ ...hostedChangeRequest("body"), updatedAt: revision })),
3127 getChangeRequestSummary: () =>
3128 Effect.gen(function* () {
3129 const result = changeRequest(1, revision);
3130 yield* Deferred.succeed(summaryStarted, undefined);
3131 yield* Deferred.await(releaseSummary);
3132 return result;
3133 }),
3134 getDiff: () =>
3135 Effect.sync(() => {
3136 diffCalls += 1;
3137 return { patch, truncated: false, nextCursor: null };
3138 }),
3139 }),
3140 ],
3141 });
3142
3143 const coldSummary = yield* service.summary(reference).pipe(Effect.forkChild());
3144 yield* Deferred.await(summaryStarted);
3145 yield* service.detail(reference);
3146 assert.strictEqual((yield* service.diff(reference)).patch, "old patch");
3147 revision = "2026-07-02T00:01:00Z";
3148 patch = "new patch";
3149 yield* TestClock.adjust("16 seconds");
3150 yield* service.detail(reference);
3151 yield* Effect.yieldNow;
3152 assert.strictEqual((yield* service.detail(reference)).updatedAt, revision);
3153 yield* Deferred.succeed(releaseSummary, undefined);
3154 yield* Fiber.join(coldSummary);
3155 yield* service.summary(reference, { recoverTransientFailure: false });
3156 assert.strictEqual((yield* service.diff(reference)).patch, "new patch");
3157 assert.strictEqual(diffCalls, 2);
3158
3159 revision = "2026-07-02T00:02:00Z";
3160 patch = "summary-discovered patch";
3161 yield* TestClock.adjust("61 seconds");
3162 yield* service.summary(reference, { recoverTransientFailure: false });
3163 assert.strictEqual((yield* service.diff(reference)).patch, patch);
3164 assert.strictEqual(diffCalls, 3);
3165 }),
3166);
3167
3060it.effect("keeps the rows when the line counts cannot be read", () =>3168it.effect("keeps the rows when the line counts cannot be read", () =>
3061 Effect.gen(function* () {3169 Effect.gen(function* () {
3062 const service = yield* makeService({3170 const service = yield* makeService({
...@@ -3080,6 +3188,7 @@ it.effect(...@@ -3080,6 +3188,7 @@ it.effect(
3080 Effect.gen(function* () {3188 Effect.gen(function* () {
3081 let coreCalls = 0;3189 let coreCalls = 0;
3082 let activityCalls = 0;3190 let activityCalls = 0;
3191 let statsCalls = 0;
3083 const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 };3192 const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 };
3084 const service = yield* makeService({3193 const service = yield* makeService({
3085 projects: [3194 projects: [
...@@ -3087,6 +3196,10 @@ it.effect(...@@ -3087,6 +3196,10 @@ it.effect(
3087 ],3196 ],
3088 providers: [3197 providers: [
3089 fakeProvider("github", {3198 fakeProvider("github", {
3199 listChangeRequestStats: () => {
3200 statsCalls += 1;
3201 return Effect.succeed([]);
3202 },
3090 getChangeRequest: () => {3203 getChangeRequest: () => {
3091 coreCalls += 1;3204 coreCalls += 1;
3092 return Effect.succeed({3205 return Effect.succeed({
...@@ -3126,6 +3239,16 @@ it.effect(...@@ -3126,6 +3239,16 @@ it.effect(
3126 assert.strictEqual(coreCalls, 1);3239 assert.strictEqual(coreCalls, 1);
3127 assert.strictEqual(activityCalls, 0);3240 assert.strictEqual(activityCalls, 0);
31283241
3242 const counts = yield* service.listStats({ refs: [reference] });
3243 assert.strictEqual(statsCalls, 0);
3244 assert.deepStrictEqual(counts.stats, [
3245 {
3246 ...reference,
3247 additions: core.additions,
3248 deletions: core.deletions,
3249 },
3250 ]);
3251
3129 yield* Effect.all([service.activity(reference), service.activity(reference)], {3252 yield* Effect.all([service.activity(reference), service.activity(reference)], {
3130 concurrency: 2,3253 concurrency: 2,
3131 });3254 });
apps/server/src/pullRequest/PullRequestService.ts+84-19
...@@ -2136,6 +2136,21 @@ export const make = Effect.gen(function* () {...@@ -2136,6 +2136,21 @@ export const make = Effect.gen(function* () {
2136 Math.max(turnRefreshEpoch, refEpochs.get(refScope(ref)) ?? 0);2136 Math.max(turnRefreshEpoch, refEpochs.get(refScope(ref)) ?? 0);
2137 const refCacheKey = (ref: PullRequestRef) =>2137 const refCacheKey = (ref: PullRequestRef) =>
2138 JSON.stringify([refEpoch(ref), ref.projectId, ref.repository, ref.number]);2138 JSON.stringify([refEpoch(ref), ref.projectId, ref.repository, ref.number]);
2139 // Counts belong to a PR, not a filtered page. Background reads and filter changes reuse
2140 // them; explicit refreshes, mutations, and turns strand old and in-flight results.
2141 const statsCacheKey = (key: string) => JSON.stringify([listingsEpoch, key]);
2142 const recentStats = new Map<
2143 string,
2144 { readonly at: number; readonly value: PullRequestDiffStat }
2145 >();
2146 const recordStats = (key: string, value: PullRequestDiffStat, at: number) => {
2147 recentStats.delete(key);
2148 recentStats.set(key, { at, value });
2149 if (recentStats.size > REF_EPOCH_CAPACITY) {
2150 const oldest = recentStats.keys().next().value;
2151 if (oldest !== undefined) recentStats.delete(oldest);
2152 }
2153 };
2139 const bumpRefEpoch = (ref: PullRequestRef) => {2154 const bumpRefEpoch = (ref: PullRequestRef) => {
2140 const scope = refScope(ref);2155 const scope = refScope(ref);
2141 if (!refEpochs.has(scope) && refEpochs.size >= REF_EPOCH_CAPACITY) {2156 if (!refEpochs.has(scope) && refEpochs.size >= REF_EPOCH_CAPACITY) {
...@@ -2175,13 +2190,15 @@ export const make = Effect.gen(function* () {...@@ -2175,13 +2190,15 @@ export const make = Effect.gen(function* () {
2175 const summary: PullRequestService["Service"]["summary"] = (input, options) => {2190 const summary: PullRequestService["Service"]["summary"] = (input, options) => {
2176 const key = refCacheKey(input);2191 const key = refCacheKey(input);
2177 const cached = Cache.get(summaryCache, key);2192 const cached = Cache.get(summaryCache, key);
2178 if (options?.recoverTransientFailure !== false) {
2179 return lastGoodSummary.serveHeld(key, cached, "reuse");
2180 }
2181 const held = lastGoodSummary.peek(key);2193 const held = lastGoodSummary.peek(key);
2182 return held?.state === "merged"2194 return held !== undefined &&
2195 (options?.recoverTransientFailure !== false || held.state === "merged")
2183 ? Effect.succeed(held)2196 ? Effect.succeed(held)
2184 : cached.pipe(Effect.tap((value) => lastGoodSummary.record(key, value)));2197 : cached.pipe(
2198 Effect.tap((value) =>
2199 shouldReplaceHeldSummary(key, value) ? lastGoodSummary.record(key, value) : Effect.void,
2200 ),
2201 );
2185 };2202 };
21862203
2187 // Keys serialize positionally and parse back in the lookup, so the cache is the only holder2204 // Keys serialize positionally and parse back in the lookup, so the cache is the only holder
...@@ -2263,8 +2280,25 @@ export const make = Effect.gen(function* () {...@@ -2263,8 +2280,25 @@ export const make = Effect.gen(function* () {
22632280
2264 const detailCache = yield* Cache.makeWith(2281 const detailCache = yield* Cache.makeWith(
2265 (key: string) => {2282 (key: string) => {
2283 const statsKey = statsCacheKey(key);
2266 const [, projectId, repository, number] = JSON.parse(key) as [number, string, string, number];2284 const [, projectId, repository, number] = JSON.parse(key) as [number, string, string, number];
2267 return detailUncached({ projectId, repository, number } as PullRequestRef);2285 return detailUncached({ projectId, repository, number } as PullRequestRef).pipe(
2286 Effect.tap(
2287 Effect.fn("PullRequestService.recordDetailStats")(function* (value: PullRequestDetail) {
2288 recordStats(
2289 statsKey,
2290 {
2291 projectId: value.projectId,
2292 repository: value.repository,
2293 number: value.number,
2294 additions: value.additions,
2295 deletions: value.deletions,
2296 },
2297 yield* Clock.currentTimeMillis,
2298 );
2299 }),
2300 ),
2301 );
2268 },2302 },
2269 {2303 {
2270 capacity: DETAIL_CACHE_CAPACITY,2304 capacity: DETAIL_CACHE_CAPACITY,
...@@ -2360,38 +2394,69 @@ export const make = Effect.gen(function* () {...@@ -2360,38 +2394,69 @@ export const make = Effect.gen(function* () {
2360 input.number,2394 input.number,
2361 input.cursor ?? null,2395 input.cursor ?? null,
2362 input.commit ?? null,2396 input.commit ?? null,
2397 input.commit === undefined
2398 ? (lastGoodSummary.peek(refCacheKey(input))?.updatedAt ?? null)
2399 : null,
2363 ]);2400 ]);
2364 return staleDiff(key, Cache.get(diffCache, key));2401 return staleDiff(key, Cache.get(diffCache, key));
2365 };2402 };
23662403
2367 const listStatsCache = yield* Cache.makeWith(2404 const listStatsCache = yield* Cache.makeWith(
2368 (key: string) => {2405 (key: string) => {
2369 const [, refs] = JSON.parse(key) as [number, ReadonlyArray<[string, string, number]>];2406 const [, refs] = JSON.parse(key) as [number, ReadonlyArray<[string, string, number, number]>];
2370 return listStatsUncached({2407 return listStatsUncached({
2371 refs: refs.map(([projectId, repository, number]) => ({ projectId, repository, number })),2408 refs: refs.map(([projectId, repository, number]) => ({ projectId, repository, number })),
2372 } as unknown as PullRequestListStatsInput);2409 } as unknown as PullRequestListStatsInput).pipe(
2410 Effect.flatMap((result) =>
2411 Clock.currentTimeMillis.pipe(Effect.map((at) => ({ result, at }))),
2412 ),
2413 );
2373 },2414 },
2374 {2415 {
2375 capacity: LIST_STATS_CACHE_CAPACITY,2416 capacity: LIST_STATS_CACHE_CAPACITY,
2376 timeToLive: (exit) => (Exit.isSuccess(exit) ? LIST_STATS_CACHE_TTL : Duration.zero),2417 timeToLive: (exit) => (Exit.isSuccess(exit) ? LIST_STATS_CACHE_TTL : Duration.zero),
2377 },2418 },
2378 );2419 );
2379 // The stats read leans on the host's search API — the scarcest limit of them all — so it2420 const statsBatchKey = (refs: Iterable<PullRequestRef>) =>
2380 // shares between clients like every other read. Refs are sorted so one page's worth of rows2421 JSON.stringify([
2381 // is one key however the client assembled them, and the listings epoch rides along so the
2382 // refresh that forgets the listing forgets its decorations with it.
2383 const listStats: PullRequestService["Service"]["listStats"] = (input) => {
2384 if (input.refs.length === 0) return Effect.succeed({ stats: [] });
2385 const key = JSON.stringify([
2386 listingsEpoch,2422 listingsEpoch,
2387 input.refs2423 [...refs]
2388 .map((ref) => [ref.projectId, ref.repository, ref.number] as const)2424 .map((ref) => [ref.projectId, ref.repository, ref.number, refEpoch(ref)] as const)
2389 .toSorted((left, right) =>2425 .toSorted((left, right) =>
2390 `${left[0]} ${left[1]} ${left[2]}`.localeCompare(`${right[0]} ${right[1]} ${right[2]}`),2426 `${left[0]} ${left[1]} ${left[2]}`.localeCompare(`${right[0]} ${right[1]} ${right[2]}`),
2391 ),2427 ),
2392 ]);2428 ]);
2393 return Cache.get(listStatsCache, key);2429 // Exact batches share in-flight reads; overlapping pages reuse each row already fetched.
2394 };2430 const listStats: PullRequestService["Service"]["listStats"] = Effect.fn(
2431 "PullRequestService.listStats",
2432 )(function* (input: PullRequestListStatsInput) {
2433 if (input.refs.length === 0) return { stats: [] };
2434 const now = yield* Clock.currentTimeMillis;
2435 const held: PullRequestDiffStat[] = [];
2436 const missing = new Map<string, PullRequestRef>();
2437 for (const ref of input.refs) {
2438 const key = statsCacheKey(refCacheKey(ref));
2439 const cached = recentStats.get(key);
2440 if (cached !== undefined && now - cached.at < Duration.toMillis(LIST_STATS_CACHE_TTL)) {
2441 held.push(cached.value);
2442 } else {
2443 missing.set(key, ref);
2444 }
2445 }
2446 if (missing.size === 0) return { stats: held };
2447 const key = statsBatchKey(missing.values());
2448 const { result, at } = yield* Cache.get(listStatsCache, key);
2449 for (const [key, ref] of missing) {
2450 const stat = result.stats.find(
2451 (stat) =>
2452 stat.projectId === ref.projectId &&
2453 stat.repository.toLowerCase() === ref.repository.toLowerCase() &&
2454 stat.number === ref.number,
2455 );
2456 if (stat !== undefined) recordStats(key, stat, at);
2457 }
2458 return { stats: [...held, ...result.stats] };
2459 });
23952460
2396 const invalidate: PullRequestService["Service"]["invalidate"] = (input) => {2461 const invalidate: PullRequestService["Service"]["invalidate"] = (input) => {
2397 const reference = input.reference;2462 const reference = input.reference;
apps/server/src/sourceControl/githubGraphQlBudget.test.ts+23-2
...@@ -9,11 +9,11 @@ const NEXT_RESET_AT = "2026-08-13T15:00:00.000Z";...@@ -9,11 +9,11 @@ const NEXT_RESET_AT = "2026-08-13T15:00:00.000Z";
9const BEFORE_RESET = Date.parse("2026-08-13T13:30:00.000Z");9const BEFORE_RESET = Date.parse("2026-08-13T13:30:00.000Z");
10const AFTER_RESET = Date.parse("2026-08-13T14:00:01.000Z");10const AFTER_RESET = Date.parse("2026-08-13T14:00:01.000Z");
1111
12function rateLimit(remaining: number, limit = 5_000, resetAt = RESET_AT): string {12function rateLimit(remaining: number, limit = 5_000, resetAt = RESET_AT, cost = 14): string {
13 return JSON.stringify({13 return JSON.stringify({
14 data: {14 data: {
15 viewer: { login: "bilal" },15 viewer: { login: "bilal" },
16 rateLimit: { cost: 14, limit, remaining, resetAt },16 rateLimit: { cost, limit, remaining, resetAt },
17 },17 },
18 });18 });
19}19}
...@@ -86,6 +86,27 @@ describe("GitHub GraphQL budget", () => {...@@ -86,6 +86,27 @@ describe("GitHub GraphQL budget", () => {
86 }).pipe(Effect.provide(GitHubGraphQlBudget.layer)),86 }).pipe(Effect.provide(GitHubGraphQlBudget.layer)),
87 );87 );
8888
89 it.effect("learns a cheaper observed cost without restoring reserved quota", () =>
90 Effect.gen(function* () {
91 yield* TestClock.setTime(BEFORE_RESET);
92 const budget = yield* GitHubGraphQlBudget.GitHubGraphQlBudget;
93 const query = "query { viewer { login } }";
94 yield* budget.observe("github.com", rateLimit(512, 5_000, RESET_AT, 8));
95 yield* budget.query("github.com", query);
96 // The admission reserved eight points, but the completed read only cost one.
97 yield* budget.observe("github.com", rateLimit(511, 5_000, RESET_AT, 1));
98
99 for (let count = 0; count < 4; count += 1) {
100 expect(yield* budget.query("github.com", query)).toContain("rateLimit");
101 }
102 const error = yield* Effect.flip(budget.query("github.com", query));
103 expect(error).toMatchObject({
104 _tag: "SourceControlRateLimitPausedError",
105 retryAt: Date.parse(RESET_AT),
106 });
107 }).pipe(Effect.provide(GitHubGraphQlBudget.layer)),
108 );
109
89 it.effect("ignores a response from an older reset window", () =>110 it.effect("ignores a response from an older reset window", () =>
90 Effect.gen(function* () {111 Effect.gen(function* () {
91 yield* TestClock.setTime(BEFORE_RESET);112 yield* TestClock.setTime(BEFORE_RESET);
apps/server/src/sourceControl/githubGraphQlBudget.ts+11-6
...@@ -122,15 +122,20 @@ export const make = Effect.gen(function* () {...@@ -122,15 +122,20 @@ export const make = Effect.gen(function* () {
122 const previous = current.get(key);122 const previous = current.get(key);
123 // Concurrent reads can finish out of order. Quota only falls within one reset window, and123 // Concurrent reads can finish out of order. Quota only falls within one reset window, and
124 // an answer from an older window must not replace the current one.124 // an answer from an older window must not replace the current one.
125 if (125 if (previous !== undefined && snapshot.resetAtMs < previous.resetAtMs) {
126 previous !== undefined &&
127 (snapshot.resetAtMs < previous.resetAtMs ||
128 (snapshot.resetAtMs === previous.resetAtMs && snapshot.remaining >= previous.remaining))
129 ) {
130 return current;126 return current;
131 }127 }
132 const next = new Map(current);128 const next = new Map(current);
133 next.set(key, snapshot);129 // Keep the conservative balance, but learn the observed cost even when our reservation
130 // was larger. Otherwise one expensive read makes every later cheap read spend its cost.
131 next.set(
132 key,
133 previous !== undefined &&
134 snapshot.resetAtMs === previous.resetAtMs &&
135 snapshot.remaining >= previous.remaining
136 ? { ...previous, cost: snapshot.cost }
137 : snapshot,
138 );
134 return next;139 return next;
135 });140 });
136 });141 });
apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx+27-23
...@@ -228,9 +228,8 @@ const TABS: ReadonlyArray<{ value: DetailTab; label: string }> = [...@@ -228,9 +228,8 @@ const TABS: ReadonlyArray<{ value: DetailTab; label: string }> = [
228 { value: "code", label: "Code" },228 { value: "code", label: "Code" },
229];229];
230230
231// The diff viewer pulls in its worker pool, so it stays out of the bundle until Code is opened.231// The diff viewer pulls in its worker pool, so load it only when the reader approaches Code.
232// Named rather than inlined so the panel can also call it itself, to start the download before232// Start the download on tab hover or focus, before the click, without loading it for every PR.
233// anyone has clicked the tab.
234const loadCodeTab = () => import("./PullRequestCodeTab");233const loadCodeTab = () => import("./PullRequestCodeTab");
235const PullRequestCodeTab = lazy(loadCodeTab);234const PullRequestCodeTab = lazy(loadCodeTab);
236235
...@@ -524,14 +523,21 @@ export function PullRequestDetailPanel({...@@ -524,14 +523,21 @@ export function PullRequestDetailPanel({
524 // summary needs it too — a large description re-parses its whole markdown on every return523 // summary needs it too — a large description re-parses its whole markdown on every return
525 // to the tab. `visibility` keeps boxes, sizes and scroll offsets, and takes hidden content524 // to the tab. `visibility` keeps boxes, sizes and scroll offsets, and takes hidden content
526 // out of the tab order and the accessibility tree.525 // out of the tab order and the accessibility tree.
527 const [mountedTabs, setMountedTabs] = useState<ReadonlySet<DetailTab>>(526 const tabScopeKey = `${environmentId}:${pullRequestKey}`;
528 () => new Set<DetailTab>(["summary"]),527 const [tabMountState, setTabMountState] = useState(() => ({
529 );528 key: tabScopeKey,
529 tabs: new Set<DetailTab>(["summary"]),
530 }));
531 // A previously visited Code tab must not fetch diffs for every later PR while hidden.
532 const mountedTabs =
533 tabMountState.key === tabScopeKey ? tabMountState.tabs : new Set<DetailTab>([tab]);
530 useEffect(() => {534 useEffect(() => {
531 setMountedTabs((previous) =>535 setTabMountState((previous) => {
532 previous.has(tab) ? previous : new Set<DetailTab>(previous).add(tab),536 if (previous.key !== tabScopeKey) return { key: tabScopeKey, tabs: new Set([tab]) };
533 );537 if (previous.tabs.has(tab)) return previous;
534 }, [tab]);538 return { key: tabScopeKey, tabs: new Set(previous.tabs).add(tab) };
539 });
540 }, [tab, tabScopeKey]);
535 const [chromeCondensed, setChromeCondensed] = useState(false);541 const [chromeCondensed, setChromeCondensed] = useState(false);
536 // Each mounted tab remembers its own scroll chrome; short tabs cannot scroll to reopen it.542 // Each mounted tab remembers its own scroll chrome; short tabs cannot scroll to reopen it.
537 const chromeStateByTab = useRef<Partial<Record<DetailTab, boolean>>>({});543 const chromeStateByTab = useRef<Partial<Record<DetailTab, boolean>>>({});
...@@ -568,12 +574,6 @@ export function PullRequestDetailPanel({...@@ -568,12 +574,6 @@ export function PullRequestDetailPanel({
568 // Which handoff is preparing, keyed so a per-finding button can say "Preparing..." on itself574 // Which handoff is preparing, keyed so a per-finding button can say "Preparing..." on itself
569 // alone. One at a time whatever the key: they all check the same pull request out.575 // alone. One at a time whatever the key: they all check the same pull request out.
570 const [handoff, setHandoff] = useState<string | null>(null);576 const [handoff, setHandoff] = useState<string | null>(null);
571 // The chunk is fetched as soon as the panel exists rather than waiting for the Code tab to be
572 // clicked, so a reader who does click it lands on a chunk already in the module cache.
573 useEffect(() => {
574 void loadCodeTab();
575 }, []);
576
577 const detailQuery = useEnvironmentQuery(577 const detailQuery = useEnvironmentQuery(
578 pullRequestEnvironment.detail({ environmentId, input: reference }),578 pullRequestEnvironment.detail({ environmentId, input: reference }),
579 );579 );
...@@ -699,12 +699,13 @@ export function PullRequestDetailPanel({...@@ -699,12 +699,13 @@ export function PullRequestDetailPanel({
699 );699 );
700 useEffect(() => {700 useEffect(() => {
701 if (!coreDetail) return;701 if (!coreDetail) return;
702 const next = { key: pullRequestKey, updatedAt: coreDetail.updatedAt };702 const next = { key: tabScopeKey, updatedAt: coreDetail.updatedAt };
703 if (shouldRefreshPullRequestActivity(activityRevision.current, next)) {703 if (shouldRefreshPullRequestActivity(activityRevision.current, next)) {
704 activityQuery.refresh();704 activityQuery.refresh();
705 setRefreshToken((token) => token + 1);
705 }706 }
706 activityRevision.current = next;707 activityRevision.current = next;
707 }, [activityQuery.refresh, coreDetail, pullRequestKey]);708 }, [activityQuery.refresh, coreDetail, tabScopeKey]);
708 useLayoutEffect(() => {709 useLayoutEffect(() => {
709 if (!resolvedCoreDetail) return;710 if (!resolvedCoreDetail) return;
710 onStateChange?.({711 onStateChange?.({
...@@ -713,16 +714,14 @@ export function PullRequestDetailPanel({...@@ -713,16 +714,14 @@ export function PullRequestDetailPanel({
713 state: resolvedCoreDetail.state,714 state: resolvedCoreDetail.state,
714 });715 });
715 }, [onStateChange, resolvedCoreDetail]);716 }, [onStateChange, resolvedCoreDetail]);
716 // Core detail is cheap enough to re-read while this stays open. Activity is heavier, so the717 // Reuse activity and diff until core detail reports a changed revision. Keyed by
717 // revision effect above reads it only after this same pull request reports a change. Keyed by
718 // the pull request rather than by the panel, because this one panel shows a different pull718 // the pull request rather than by the panel, because this one panel shows a different pull
719 // request every time it is opened.719 // request every time it is opened.
720 useLiveRefresh(720 useLiveRefresh(
721 () => {721 () => {
722 detailQuery.refresh();722 detailQuery.refresh();
723 setRefreshToken((token) => token + 1);
724 },723 },
725 { key: `pull-request:${reference.projectId}:${reference.repository}#${reference.number}` },724 { key: `pull-request:${environmentId}:${pullRequestKey}` },
726 );725 );
727 // The button, on the other hand, goes around the server's cache rather than through it: it is726 // The button, on the other hand, goes around the server's cache rather than through it: it is
728 // the answer for a reader who can see that what they are looking at is behind. The727 // the answer for a reader who can see that what they are looking at is behind. The
...@@ -2143,7 +2142,12 @@ export function PullRequestDetailPanel({...@@ -2143,7 +2142,12 @@ export function PullRequestDetailPanel({
2143 }}2142 }}
2144 >2143 >
2145 {visibleTabs.map((item) => (2144 {visibleTabs.map((item) => (
2146 <Toggle key={item.value} value={item.value}>2145 <Toggle
2146 key={item.value}
2147 value={item.value}
2148 onPointerEnter={item.value === "code" ? () => void loadCodeTab() : undefined}
2149 onFocus={item.value === "code" ? () => void loadCodeTab() : undefined}
2150 >
2147 {item.label}2151 {item.label}
2148 </Toggle>2152 </Toggle>
2149 ))}2153 ))}
apps/web/src/components/pullRequest/PullRequestRow.tsx+35-35
...@@ -97,11 +97,11 @@ function PullRequestRowImpl({...@@ -97,11 +97,11 @@ function PullRequestRowImpl({
97 aria-current={selected ? "true" : undefined}97 aria-current={selected ? "true" : undefined}
98 onClick={() => onSelect(entry)}98 onClick={() => onSelect(entry)}
99 className={cn(99 className={cn(
100 "grid w-full grid-cols-[auto_minmax(0,1fr)] items-center gap-3 rounded-lg px-3 py-2 text-left transition-colors focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring",100 "@container/pr-row grid w-full grid-cols-[auto_minmax(0,1fr)] items-center gap-3 rounded-lg px-3 py-3 text-left transition-colors focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring",
101 // Offscreen rows are skipped for style, layout and paint: a long list costs what the101 // Offscreen rows are skipped for style, layout and paint: a long list costs what the
102 // viewport shows, not what the pages have loaded. The intrinsic size keeps the102 // viewport shows, not what the pages have loaded. The intrinsic size keeps the
103 // scrollbar honest while a row is skipped.103 // scrollbar honest while a row is skipped.
104 "[contain-intrinsic-block-size:54px] [content-visibility:auto]",104 "[contain-intrinsic-block-size:66px] [content-visibility:auto]",
105 selected ? "bg-accent" : "hover:bg-accent/60",105 selected ? "bg-accent" : "hover:bg-accent/60",
106 )}106 )}
107 >107 >
...@@ -111,12 +111,36 @@ function PullRequestRowImpl({...@@ -111,12 +111,36 @@ function PullRequestRowImpl({
111 mergeability={entry.mergeability}111 mergeability={entry.mergeability}
112 baseBranch={entry.baseBranch}112 baseBranch={entry.baseBranch}
113 />113 />
114 <span className="grid min-w-0 grid-cols-[minmax(0,1fr)_auto] items-center gap-x-3 gap-y-0.5">114 <span className="grid min-w-0 grid-cols-[minmax(0,1fr)_auto] items-center gap-x-3 gap-y-1.5">
115 <span className="col-start-1 row-start-1 block truncate text-sm font-medium text-foreground">115 <span className="col-start-1 row-start-1 block truncate text-sm font-medium text-foreground">
116 {entry.title}116 {entry.title}
117 </span>117 </span>
118 <span className="col-start-2 row-start-1 justify-self-end whitespace-nowrap text-xs text-muted-foreground/70 tabular-nums">118 <span className="col-start-2 row-start-1 flex max-w-36 items-center justify-self-end gap-2 text-xs">
119 {formatRelativeTimeLabel(entry.updatedAt)}119 {/* Only a verdict somebody has actually given: "review required" is the absence of
120 one, and saying so on every unreviewed row would say nothing. */}
121 {entry.reviewDecision === "approved" || entry.reviewDecision === "changes-requested" ? (
122 <span
123 className={cn(
124 "sr-only @md/pr-row:not-sr-only @md/pr-row:min-w-0 @md/pr-row:truncate",
125 entry.reviewDecision === "approved"
126 ? "text-emerald-600/90 dark:text-emerald-400/80"
127 : "text-amber-600/90 dark:text-amber-400/80",
128 )}
129 >
130 {entry.reviewDecision === "approved" ? "Approved" : "Changes requested"}
131 </span>
132 ) : null}
133 {entry.checksState === undefined ? null : (
134 <PullRequestChecksPopover
135 checksState={entry.checksState}
136 environmentId={entry.environmentId}
137 reference={{
138 projectId: entry.projectId,
139 repository: entry.repository,
140 number: entry.number,
141 }}
142 />
143 )}
120 </span>144 </span>
121 <PullRequestMetaLine className="@container/pr-row-meta col-start-1 row-start-2 overflow-hidden text-xs text-muted-foreground/70">145 <PullRequestMetaLine className="@container/pr-row-meta col-start-1 row-start-2 overflow-hidden text-xs text-muted-foreground/70">
122 {matchedElsewhere ? (146 {matchedElsewhere ? (
...@@ -170,37 +194,13 @@ function PullRequestRowImpl({...@@ -170,37 +194,13 @@ function PullRequestRowImpl({
170 labelClassName="sr-only @xs/pr-row-meta:not-sr-only @xs/pr-row-meta:truncate"194 labelClassName="sr-only @xs/pr-row-meta:not-sr-only @xs/pr-row-meta:truncate"
171 />195 />
172 {entry.labels.length > 0 ? <PullRequestRowLabels labels={entry.labels} /> : null}196 {entry.labels.length > 0 ? <PullRequestRowLabels labels={entry.labels} /> : null}
173 {/* Only a verdict somebody has actually given: "review required" is the absence of
174 one, and saying so on every unreviewed row would say nothing. */}
175 {entry.reviewDecision === "approved" || entry.reviewDecision === "changes-requested" ? (
176 <span
177 className={cn(
178 "min-w-0 truncate",
179 entry.reviewDecision === "approved"
180 ? "text-emerald-600/90 dark:text-emerald-400/80"
181 : "text-amber-600/90 dark:text-amber-400/80",
182 )}
183 >
184 {entry.reviewDecision === "approved" ? "Approved" : "Changes requested"}
185 </span>
186 ) : null}
187 {entry.checksState === undefined ? null : (
188 <PullRequestChecksPopover
189 checksState={entry.checksState}
190 environmentId={entry.environmentId}
191 reference={{
192 projectId: entry.projectId,
193 repository: entry.repository,
194 number: entry.number,
195 }}
196 />
197 )}
198 </PullRequestMetaLine>197 </PullRequestMetaLine>
199 <PullRequestDiffStat198 <span className="col-start-2 row-start-2 flex items-center justify-self-end gap-3 whitespace-nowrap text-[11px] text-muted-foreground/70 tabular-nums">
200 additions={entry.additions}199 <PullRequestDiffStat additions={entry.additions} deletions={entry.deletions} />
201 deletions={entry.deletions}200 <span className="hidden @sm/pr-row:inline">
202 className="col-start-2 row-start-2 justify-self-end text-xs"201 {formatRelativeTimeLabel(entry.updatedAt)}
203 />202 </span>
203 </span>
204 </span>204 </span>
205 </button>205 </button>
206 );206 );
apps/web/src/components/pullRequest/pullRequestList.logic.test.ts+54-11
...@@ -69,8 +69,30 @@ function entry(...@@ -69,8 +69,30 @@ function entry(
69}69}
7070
71describe("visible pull request line-count targets", () => {71describe("visible pull request line-count targets", () => {
72 it("reuses counts supplied by the listing and only requests missing counts", () => {
73 const entries = [
74 entry({ number: 1, additions: 12, deletions: 0 }),
75 entry({ number: 2, additions: 0, deletions: 7 }),
76 entry({ number: 3, additions: 0, deletions: 0 }),
77 ];
78 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));
79 const keys = pullRequestStatsKeysToRequest(
80 entriesByKey,
81 new Set(entries.map(pullRequestEntryKey)),
82 [],
83 new Map(),
84 );
85
86 expect(pullRequestStatsBatches(entriesByKey, keys)[0]?.input.refs).toEqual([
87 { projectId: "project-1", repository: "pingdotgg/t3code", number: 3 },
88 ]);
89 });
90
72 it("does not request a row again after its received batch is pruned", () => {91 it("does not request a row again after its received batch is pruned", () => {
73 const entries = [entry({ number: 1 }), entry({ number: 2 })];92 const entries = [
93 entry({ additions: 0, deletions: 0, number: 1 }),
94 entry({ additions: 0, deletions: 0, number: 2 }),
95 ];
74 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));96 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));
75 const firstKey = pullRequestEntryKey(entries[0]!);97 const firstKey = pullRequestEntryKey(entries[0]!);
76 const secondKey = pullRequestEntryKey(entries[1]!);98 const secondKey = pullRequestEntryKey(entries[1]!);
...@@ -97,7 +119,9 @@ describe("visible pull request line-count targets", () => {...@@ -97,7 +119,9 @@ describe("visible pull request line-count targets", () => {
97 });119 });
98120
99 it("drops historical rows after a long scroll so refresh stays bounded to the viewport", () => {121 it("drops historical rows after a long scroll so refresh stays bounded to the viewport", () => {
100 const entries = Array.from({ length: 500 }, (_, index) => entry({ number: index + 1 }));122 const entries = Array.from({ length: 500 }, (_, index) =>
123 entry({ additions: 0, deletions: 0, number: index + 1 }),
124 );
101 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));125 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));
102 const batches = entries.map(126 const batches = entries.map(
103 (item) => pullRequestStatsBatches(entriesByKey, new Set([pullRequestEntryKey(item)]))[0]!,127 (item) => pullRequestStatsBatches(entriesByKey, new Set([pullRequestEntryKey(item)]))[0]!,
...@@ -112,7 +136,9 @@ describe("visible pull request line-count targets", () => {...@@ -112,7 +136,9 @@ describe("visible pull request line-count targets", () => {
112 });136 });
113137
114 it("keeps every per-environment batch within the stats contract limit", () => {138 it("keeps every per-environment batch within the stats contract limit", () => {
115 const entries = Array.from({ length: 501 }, (_, index) => entry({ number: index + 1 }));139 const entries = Array.from({ length: 501 }, (_, index) =>
140 entry({ additions: 0, deletions: 0, number: index + 1 }),
141 );
116 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));142 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));
117 const batches = pullRequestStatsBatches(143 const batches = pullRequestStatsBatches(
118 entriesByKey,144 entriesByKey,
...@@ -125,9 +151,9 @@ describe("visible pull request line-count targets", () => {...@@ -125,9 +151,9 @@ describe("visible pull request line-count targets", () => {
125151
126 it("selects visible rows for date modes and every uncached row for size modes", () => {152 it("selects visible rows for date modes and every uncached row for size modes", () => {
127 const entries = [153 const entries = [
128 entry({ number: 1 }),154 entry({ additions: 0, deletions: 0, number: 1 }),
129 entry({ number: 2 }),155 entry({ additions: 0, deletions: 0, number: 2 }),
130 entry({ number: 3, environmentId: "env-2" as EnvironmentId }),156 entry({ additions: 0, deletions: 0, number: 3, environmentId: "env-2" as EnvironmentId }),
131 ];157 ];
132 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));158 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));
133 const [firstKey, secondKey] = entries.map(pullRequestEntryKey);159 const [firstKey, secondKey] = entries.map(pullRequestEntryKey);
...@@ -162,7 +188,11 @@ describe("visible pull request line-count targets", () => {...@@ -162,7 +188,11 @@ describe("visible pull request line-count targets", () => {
162 });188 });
163189
164 it("refreshes only visible rows unless size sorting needs every loaded row", () => {190 it("refreshes only visible rows unless size sorting needs every loaded row", () => {
165 const entries = [entry({ number: 1 }), entry({ number: 2 }), entry({ number: 3 })];191 const entries = [
192 entry({ additions: 0, deletions: 0, number: 1 }),
193 entry({ additions: 0, deletions: 0, number: 2 }),
194 entry({ additions: 0, deletions: 0, number: 3 }),
195 ];
166 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));196 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));
167 const visibleKeys = new Set([pullRequestEntryKey(entries[1]!)]);197 const visibleKeys = new Set([pullRequestEntryKey(entries[1]!)]);
168 const cachedStats = mergePullRequestDiffStats(198 const cachedStats = mergePullRequestDiffStats(
...@@ -198,7 +228,10 @@ describe("visible pull request line-count targets", () => {...@@ -198,7 +228,10 @@ describe("visible pull request line-count targets", () => {
198 });228 });
199229
200 it("ignores a late refresh after the filter or stats policy changes", () => {230 it("ignores a late refresh after the filter or stats policy changes", () => {
201 const entries = [entry({ number: 1 }), entry({ number: 2 })];231 const entries = [
232 entry({ additions: 0, deletions: 0, number: 1 }),
233 entry({ additions: 0, deletions: 0, number: 2 }),
234 ];
202 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));235 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));
203 const visibleKeys = new Set([pullRequestEntryKey(entries[1]!)]);236 const visibleKeys = new Set([pullRequestEntryKey(entries[1]!)]);
204 const requestedScope = { key: "open", policy: "visible" } as const;237 const requestedScope = { key: "open", policy: "visible" } as const;
...@@ -217,7 +250,10 @@ describe("visible pull request line-count targets", () => {...@@ -217,7 +250,10 @@ describe("visible pull request line-count targets", () => {
217 });250 });
218251
219 it("does not add request batches again while rows are active or cached", () => {252 it("does not add request batches again while rows are active or cached", () => {
220 const entries = [entry({ number: 1 }), entry({ number: 2 })];253 const entries = [
254 entry({ additions: 0, deletions: 0, number: 1 }),
255 entry({ additions: 0, deletions: 0, number: 2 }),
256 ];
221 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));257 const entriesByKey = new Map(entries.map((item) => [pullRequestEntryKey(item), item]));
222 const first = pullRequestStatsRequestBatches({258 const first = pullRequestStatsRequestBatches({
223 entriesByKey,259 entriesByKey,
...@@ -724,7 +760,7 @@ describe("default merge-readiness ranking", () => {...@@ -724,7 +760,7 @@ describe("default merge-readiness ranking", () => {
724 ]);760 ]);
725 });761 });
726762
727 it("puts the smallest measured change first inside a readiness tier", () => {763 it("keeps readiness order stable when optional diff counts arrive", () => {
728 const larger = entry({764 const larger = entry({
729 number: 1,765 number: 1,
730 checksState: "passing",766 checksState: "passing",
...@@ -752,7 +788,14 @@ describe("default merge-readiness ranking", () => {...@@ -752,7 +788,14 @@ describe("default merge-readiness ranking", () => {
752788
753 expect(789 expect(
754 rankPullRequestsByMergeReadiness([larger, unknown, smaller]).map((row) => row.number),790 rankPullRequestsByMergeReadiness([larger, unknown, smaller]).map((row) => row.number),
755 ).toEqual([2, 1, 3]);791 ).toEqual([3, 1, 2]);
792 expect(
793 rankPullRequestsByMergeReadiness([
794 larger,
795 { ...unknown, additions: 500, deletions: 200 },
796 smaller,
797 ]).map((row) => row.number),
798 ).toEqual([3, 1, 2]);
756 });799 });
757800
758 it("keeps authored work first and ranks each group by readiness", () => {801 it("keeps authored work first and ranks each group by readiness", () => {
apps/web/src/components/pullRequest/pullRequestList.logic.ts+8-8
...@@ -469,7 +469,11 @@ export function pullRequestStatsKeysToRequest(...@@ -469,7 +469,11 @@ export function pullRequestStatsKeysToRequest(
469 [...enteredKeys].filter((key) => {469 [...enteredKeys].filter((key) => {
470 const entry = entriesByKey.get(key);470 const entry = entriesByKey.get(key);
471 return (471 return (
472 entry !== undefined && !requested.has(key) && !statsByRow.has(pullRequestDiffStatKey(entry))472 entry !== undefined &&
473 entry.additions === 0 &&
474 entry.deletions === 0 &&
475 !requested.has(key) &&
476 !statsByRow.has(pullRequestDiffStatKey(entry))
473 );477 );
474 }),478 }),
475 );479 );
...@@ -1006,11 +1010,10 @@ export function rankPullRequestMatches<Entry extends PullRequestListEntry>(...@@ -1006,11 +1010,10 @@ export function rankPullRequestMatches<Entry extends PullRequestListEntry>(
1006 * verdict, then everything else still open. Drafts stay in that third tier because their author1010 * verdict, then everything else still open. Drafts stay in that third tier because their author
1007 * has not made them mergeable yet. Finished work follows open work when all states are visible. A1011 * has not made them mergeable yet. Finished work follows open work when all states are visible. A
1008 * known conflict is never ready, whatever its checks, review or state say, so it stays at the1012 * known conflict is never ready, whatever its checks, review or state say, so it stays at the
1009 * bottom. Smaller measured changes come first within a tier; recency only breaks a remaining tie.1013 * bottom. Recency breaks ties, so optional diff counts never move the queue beneath the reader.
1010 */1014 */
1011export function rankPullRequestsByMergeReadiness<Entry extends PullRequestListEntry>(1015export function rankPullRequestsByMergeReadiness<Entry extends PullRequestListEntry>(
1012 entries: ReadonlyArray<Entry>,1016 entries: ReadonlyArray<Entry>,
1013 hasMeasuredSize: (entry: Entry) => boolean = (entry) => entry.additions + entry.deletions > 0,
1014): ReadonlyArray<Entry> {1017): ReadonlyArray<Entry> {
1015 const tier = (entry: Entry) => {1018 const tier = (entry: Entry) => {
1016 if (entry.mergeability === "conflicting") return 4;1019 if (entry.mergeability === "conflicting") return 4;
...@@ -1023,10 +1026,7 @@ export function rankPullRequestsByMergeReadiness<Entry extends PullRequestListEn...@@ -1023,10 +1026,7 @@ export function rankPullRequestsByMergeReadiness<Entry extends PullRequestListEn
1023 return entries.toSorted((left, right) => {1026 return entries.toSorted((left, right) => {
1024 const byTier = tier(left) - tier(right);1027 const byTier = tier(left) - tier(right);
1025 if (byTier !== 0) return byTier;1028 if (byTier !== 0) return byTier;
1026 const byMeasurement = Number(hasMeasuredSize(right)) - Number(hasMeasuredSize(left));1029 return right.updatedAt.localeCompare(left.updatedAt);
1027 if (byMeasurement !== 0) return byMeasurement;
1028 const bySize = left.additions + left.deletions - (right.additions + right.deletions);
1029 return bySize !== 0 ? bySize : right.updatedAt.localeCompare(left.updatedAt);
1030 });1030 });
1031}1031}
10321032
...@@ -1042,7 +1042,7 @@ export function sortPullRequestGroups<Entry extends PullRequestListEntry>(...@@ -1042,7 +1042,7 @@ export function sortPullRequestGroups<Entry extends PullRequestListEntry>(
10421042
1043 if (sort === "ready") {1043 if (sort === "ready") {
1044 return searchText.trim().length === 01044 return searchText.trim().length === 0
1045 ? sortWithinGroups((entries) => rankPullRequestsByMergeReadiness(entries, hasMeasuredSize))1045 ? sortWithinGroups(rankPullRequestsByMergeReadiness)
1046 : groups;1046 : groups;
1047 }1047 }
1048 if (sort === "updated") return groups;1048 if (sort === "updated") return groups;
apps/web/src/routes/_chat.pull-requests.tsx+48-75
...@@ -292,7 +292,7 @@ function PullRequestsRouteView() {...@@ -292,7 +292,7 @@ function PullRequestsRouteView() {
292 const search = Route.useSearch();292 const search = Route.useSearch();
293 const sort = search.sort ?? "ready";293 const sort = search.sort ?? "ready";
294 const statsPolicy: PullRequestStatsPolicy =294 const statsPolicy: PullRequestStatsPolicy =
295 sort === "ready" || sort === "largest" || sort === "smallest" ? "eager" : "visible";295 sort === "largest" || sort === "smallest" ? "eager" : "visible";
296 const navigate = useNavigate({ from: Route.fullPath });296 const navigate = useNavigate({ from: Route.fullPath });
297 const keybindings = useAtomValue(primaryServerKeybindingsAtom);297 const keybindings = useAtomValue(primaryServerKeybindingsAtom);
298 const { environments } = useEnvironments();298 const { environments } = useEnvironments();
...@@ -838,11 +838,7 @@ function PullRequestsRouteView() {...@@ -838,11 +838,7 @@ function PullRequestsRouteView() {
838 } finally {838 } finally {
839 setInvalidating(false);839 setInvalidating(false);
840 }840 }
841 refreshList();841 refreshList(true);
842 baselineQuery.refresh();
843 facetQuery.refresh();
844 authoredQuery.refresh();
845 reviewingQuery.refresh();
846 const visible = visibleStatsKeys.current;842 const visible = visibleStatsKeys.current;
847 const batches = pullRequestStatsRefreshBatches({843 const batches = pullRequestStatsRefreshBatches({
848 requestedScope: requestedStatsScope,844 requestedScope: requestedStatsScope,
...@@ -955,7 +951,7 @@ function PullRequestsRouteView() {...@@ -955,7 +951,7 @@ function PullRequestsRouteView() {
955 environmentKey,951 environmentKey,
956 scope: scopeKey,952 scope: scopeKey,
957 query: sentQuery,953 query: sentQuery,
958 data,954 data: { ...data, entries: ordered?.key === filterKey ? ordered.entries : data.entries },
959 ...(partitions === undefined ? {} : { partitions }),955 ...(partitions === undefined ? {} : { partitions }),
960 };956 };
961 });957 });
...@@ -1017,7 +1013,7 @@ function PullRequestsRouteView() {...@@ -1017,7 +1013,7 @@ function PullRequestsRouteView() {
1017 // `ordered` is declared above, ahead of the snapshot write, but grown here from this round's1013 // `ordered` is declared above, ahead of the snapshot write, but grown here from this round's
1018 // own answer.1014 // own answer.
1019 useEffect(() => {1015 useEffect(() => {
1020 if (!answered) return;1016 if (!answered || listQuery.isPending || (listQuery.error && listQuery.data === null)) return;
1021 setOrdered((previous) => {1017 setOrdered((previous) => {
1022 if (previous === null || previous.key !== filterKey) {1018 if (previous === null || previous.key !== filterKey) {
1023 return {1019 return {
...@@ -1045,7 +1041,15 @@ function PullRequestsRouteView() {...@@ -1045,7 +1041,15 @@ function PullRequestsRouteView() {
1045 // reads, so its order stands; a row that moved was updated, and moving is the news.1041 // reads, so its order stands; a row that moved was updated, and moving is the news.
1046 return { key: filterKey, entries: rankPullRequestMatches(answered.entries, sentParsed.text) };1042 return { key: filterKey, entries: rankPullRequestMatches(answered.entries, sentParsed.text) };
1047 });1043 });
1048 }, [answered, filterKey, sentCursors, sentParsed.text]);1044 }, [
1045 answered,
1046 filterKey,
1047 sentCursors,
1048 sentParsed.text,
1049 listQuery.isPending,
1050 listQuery.error,
1051 listQuery.data,
1052 ]);
10491053
1050 // Carrying on where the last answer stopped, and only raising the page size for the hosts that1054 // Carrying on where the last answer stopped, and only raising the page size for the hosts that
1051 // could not say where that was.1055 // could not say where that was.
...@@ -1081,11 +1085,20 @@ function PullRequestsRouteView() {...@@ -1081,11 +1085,20 @@ function PullRequestsRouteView() {
1081 // re-reads only its own slice, so the rows loaded before it would never see a merge, a close,1085 // re-reads only its own slice, so the rows loaded before it would never see a merge, a close,
1082 // or a retitle. Going back to a single page long enough to cover everything on screen lets the1086 // or a retitle. Going back to a single page long enough to cover everything on screen lets the
1083 // merge above bring every row up to date in place.1087 // merge above bring every row up to date in place.
1084 const refreshList = () => {1088 const refreshList = (includeRelated = false) => {
1089 const related = includeRelated
1090 ? [
1091 ...baselineTargets,
1092 ...facetTargets,
1093 ...partitionTargets.authored,
1094 ...partitionTargets.reviewing,
1095 ]
1096 : [];
1085 if (sentCursors === null) {1097 if (sentCursors === null) {
1086 listQuery.refresh();1098 listQuery.refresh([...listTargets, ...related]);
1087 return;1099 return;
1088 }1100 }
1101 if (related.length > 0) listQuery.refresh(related);
1089 const loadedCount = ordered?.key === filterKey ? ordered.entries.length : pageSize;1102 const loadedCount = ordered?.key === filterKey ? ordered.entries.length : pageSize;
1090 setPage({1103 setPage({
1091 key: filterKey,1104 key: filterKey,
...@@ -1117,9 +1130,7 @@ function PullRequestsRouteView() {...@@ -1117,9 +1130,7 @@ function PullRequestsRouteView() {
1117 // host's rate limit.1130 // host's rate limit.
1118 useLiveRefresh(1131 useLiveRefresh(
1119 () => {1132 () => {
1120 refreshList();1133 refreshList(true);
1121 authoredQuery.refresh();
1122 reviewingQuery.refresh();
1123 },1134 },
1124 { enabled: pullRequestsSupported },1135 { enabled: pullRequestsSupported },
1125 );1136 );
...@@ -1209,54 +1220,6 @@ function PullRequestsRouteView() {...@@ -1209,54 +1220,6 @@ function PullRequestsRouteView() {
1209 );1220 );
12101221
1211 const scrollRef = useRef<HTMLDivElement>(null);1222 const scrollRef = useRef<HTMLDivElement>(null);
1212 const sentinelRef = useRef<HTMLDivElement>(null);
1213 useEffect(() => {
1214 const sentinel = sentinelRef.current;
1215 // A failed page must stop the observer. Retained rows keep the sentinel on screen, so
1216 // re-arming it after a failure would ask for the next page again, forever.
1217 //
1218 // Rows on screen are also what makes reaching the sentinel mean anything: with none, it
1219 // sits directly below the empty state and is always in view, so a search that matches
1220 // nothing would page through the whole host on its own — one listing of every repository
1221 // per step — while the reader looks at an empty page. With nothing to scroll past, the
1222 // next page is asked for rather than assumed.
1223 if (
1224 !sentinel ||
1225 entries.length === 0 ||
1226 listData?.truncated !== true ||
1227 listQuery.isPending ||
1228 listQuery.error !== null ||
1229 // The rows on screen belong to the previous question, so nothing about them says where
1230 // this one carries on from. Growing the page under them would answer neither.
1231 showingCarried ||
1232 // Asking past the cap is refused, which would strand the list on an error the retry
1233 // could never clear, so growth stops here and the rest stays on the host. A continuation
1234 // does not grow the page at all, so the cap does not apply to it.
1235 (!canContinue && pageSize >= MAX_PAGE_SIZE)
1236 ) {
1237 return;
1238 }
1239 const observer = new IntersectionObserver(
1240 (observed) => {
1241 if (observed.some((entry) => entry.isIntersecting)) {
1242 loadMore();
1243 }
1244 },
1245 // Start the next page slightly before the sentinel is on screen.
1246 { root: scrollRef.current, rootMargin: "240px" },
1247 );
1248 observer.observe(sentinel);
1249 return () => observer.disconnect();
1250 }, [
1251 entries.length,
1252 filterKey,
1253 canContinue,
1254 listData?.truncated,
1255 listQuery.error,
1256 listQuery.isPending,
1257 pageSize,
1258 showingCarried,
1259 ]);
12601223
1261 /**1224 /**
1262 * The line counts, asked for once the rows are on screen. On GitHub they are forty per cent of1225 * The line counts, asked for once the rows are on screen. On GitHub they are forty per cent of
...@@ -1633,7 +1596,7 @@ function PullRequestsRouteView() {...@@ -1633,7 +1596,7 @@ function PullRequestsRouteView() {
1633 />1596 />
1634 ) : firstLoad ? (1597 ) : firstLoad ? (
1635 <PullRequestListGhost rows={7} />1598 <PullRequestListGhost rows={7} />
1636 ) : listQuery.error && listData === null ? (1599 ) : listQuery.error && entries.length === 0 ? (
1637 <PullRequestsUnavailableState error={listQuery.error} onRetry={() => listQuery.refresh()} />1600 <PullRequestsUnavailableState error={listQuery.error} onRetry={() => listQuery.refresh()} />
1638 ) : carriedToNothing ? (1601 ) : carriedToNothing ? (
1639 <PullRequestListGhost rows={7} />1602 <PullRequestListGhost rows={7} />
...@@ -1699,22 +1662,33 @@ function PullRequestsRouteView() {...@@ -1699,22 +1662,33 @@ function PullRequestsRouteView() {
1699 </div>1662 </div>
1700 )}1663 )}
17011664
1702 {listQuery.error && listData !== null ? (1665 {listQuery.error && entries.length > 0 ? (
1703 <div className="flex items-center justify-between gap-3 rounded-lg border border-amber-500/30 bg-amber-500/5 px-3 py-2 text-xs">1666 <div className="flex items-center justify-between gap-3 rounded-lg border border-amber-500/30 bg-amber-500/5 px-3 py-2 text-xs">
1704 <span>The latest request failed. Showing the last pull requests loaded.</span>1667 <span>{listQuery.error} Showing the last pull requests loaded.</span>
1705 <Button size="xs" variant="outline" onClick={() => listQuery.refresh()}>1668 <Button size="xs" variant="outline" onClick={() => listQuery.refresh()}>
1706 Retry1669 Retry
1707 </Button>1670 </Button>
1708 </div>1671 </div>
1709 ) : null}1672 ) : null}
1710 {listData?.truncated && entries.length > 0 ? (1673 {listData?.truncated && entries.length > 0 ? (
1711 <div ref={sentinelRef} className="flex justify-center py-2 text-xs text-muted-foreground">1674 <div className="flex justify-center py-3 text-xs text-muted-foreground">
1712 {loadingMore ? (1675 {loadingMore ? (
1713 <span className="flex items-center gap-2">1676 <span className="flex items-center gap-2">
1714 <LoaderIcon aria-hidden className="size-3.5 animate-spin" />1677 <LoaderIcon aria-hidden className="size-3.5 animate-spin" />
1715 Loading more1678 {sentCursors === null ? "Updating pull requests" : "Loading more"}
1716 </span>1679 </span>
1717 ) : null}1680 ) : canContinue || pageSize < MAX_PAGE_SIZE ? (
1681 <Button
1682 size="sm"
1683 variant="outline"
1684 onClick={loadMore}
1685 disabled={listQuery.isPending || showingCarried}
1686 >
1687 Load more pull requests
1688 </Button>
1689 ) : (
1690 <span>Narrow your search to find more pull requests.</span>
1691 )}
1718 </div>1692 </div>
1719 ) : null}1693 ) : null}
1720 </>1694 </>
...@@ -1979,10 +1953,7 @@ function PullRequestsRouteView() {...@@ -1979,10 +1953,7 @@ function PullRequestsRouteView() {
1979 // Merging, closing or reopening changes the row this panel was opened from, so1953 // Merging, closing or reopening changes the row this panel was opened from, so
1980 // the list behind it is out of date the moment the host takes the action.1954 // the list behind it is out of date the moment the host takes the action.
1981 onActed={() => {1955 onActed={() => {
1982 refreshList();1956 refreshList(true);
1983 baselineQuery.refresh();
1984 authoredQuery.refresh();
1985 reviewingQuery.refresh();
1986 }}1957 }}
1987 />1958 />
1988 </RightPanelTabs>1959 </RightPanelTabs>
...@@ -2244,7 +2215,7 @@ function PullRequestsColumn({...@@ -2244,7 +2215,7 @@ function PullRequestsColumn({
2244 return (2215 return (
2245 // Painted flat like the chat column: the inset underneath carries the chrome grain, and a2216 // Painted flat like the chat column: the inset underneath carries the chrome grain, and a
2246 // content surface that lets it show reads as a different background than every thread.2217 // content surface that lets it show reads as a different background than every thread.
2247 <div className="flex min-h-0 min-w-0 flex-1 flex-col bg-background">2218 <div className="@container/pr-list flex min-h-0 min-w-0 flex-1 flex-col bg-background">
2248 {/* A closed right panel leaves this column full-width, so the shared header2219 {/* A closed right panel leaves this column full-width, so the shared header
2249 reserves native window controls and hosts the controls strip itself: on2220 reserves native window controls and hosts the controls strip itself: on
2250 desktop the header is a drag-region, and only a no-drag descendant wins2221 desktop the header is a drag-region, and only a no-drag descendant wins
...@@ -2325,8 +2296,10 @@ function PullRequestsColumn({...@@ -2325,8 +2296,10 @@ function PullRequestsColumn({
2325 content actually passing under the chrome fades. */}2296 content actually passing under the chrome fades. */}
2326 <WorkspacePageContainer width="expanded" className="gap-4">2297 <WorkspacePageContainer width="expanded" className="gap-4">
2327 <div className="flex flex-col gap-3">2298 <div className="flex flex-col gap-3">
2328 <div ref={inFlowSearchRef} className="flex items-center gap-2">2299 <div ref={inFlowSearchRef} className="flex flex-wrap items-center gap-2">
2329 {searchInput}2300 <div className="min-w-0 basis-full @lg/pr-list:basis-0 @lg/pr-list:flex-1">
2301 {searchInput}
2302 </div>
2330 {sortMenu}2303 {sortMenu}
2331 {filtersMenu}2304 {filtersMenu}
2332 {!condensed ? (2305 {!condensed ? (
apps/web/src/state/pullRequests.ts+3-3
...@@ -130,8 +130,8 @@ function createMergedEnvironmentQuery<Input, A>(...@@ -130,8 +130,8 @@ function createMergedEnvironmentQuery<Input, A>(
130 (override?: ReadonlyArray<EnvironmentQueryTarget<Input>>) => {130 (override?: ReadonlyArray<EnvironmentQueryTarget<Input>>) => {
131 const refreshTargets =131 const refreshTargets =
132 override ?? (JSON.parse(key) as ReadonlyArray<EnvironmentQueryTarget<Input>>);132 override ?? (JSON.parse(key) as ReadonlyArray<EnvironmentQueryTarget<Input>>);
133 for (const target of refreshTargets) {133 for (const atom of new Set(refreshTargets.map(atomFor))) {
134 appAtomRegistry.refresh(atomFor(target));134 appAtomRegistry.refresh(atom);
135 }135 }
136 },136 },
137 [key],137 [key],
...@@ -173,7 +173,7 @@ export interface MergedPullRequestListView {...@@ -173,7 +173,7 @@ export interface MergedPullRequestListView {
173 readonly data: MergedPullRequestList | null;173 readonly data: MergedPullRequestList | null;
174 readonly error: string | null;174 readonly error: string | null;
175 readonly isPending: boolean;175 readonly isPending: boolean;
176 readonly refresh: () => void;176 readonly refresh: (targets?: ReadonlyArray<EnvironmentQueryTarget<PullRequestListInput>>) => void;
177}177}
178178
179/** One listing per environment, merged into the single list the page renders. */179/** One listing per environment, merged into the single list the page renders. */
packages/client-runtime/src/state/pullRequests.ts+4-4
...@@ -75,8 +75,8 @@ export function pullRequestDetailToVcsStatus(...@@ -75,8 +75,8 @@ export function pullRequestDetailToVcsStatus(
75}75}
7676
77/**77/**
78 * Every read shells out to the GitHub CLI, so results are reused for a short while and78 * Reopening a PR within a minute reuses detail and activity. Explicit refreshes and
79 * refreshed explicitly. Mutations run serially per environment: `gh` actions on the same79 * turn notifications still revalidate. Mutations run serially per environment: actions on the same
80 * pull request are order-sensitive, and the detail view refetches after each one.80 * pull request are order-sensitive, and the detail view refetches after each one.
81 */81 */
82export function createPullRequestEnvironmentAtoms<R, E>(82export function createPullRequestEnvironmentAtoms<R, E>(
...@@ -91,7 +91,7 @@ export function createPullRequestEnvironmentAtoms<R, E>(...@@ -91,7 +91,7 @@ export function createPullRequestEnvironmentAtoms<R, E>(
91 const activity = createEnvironmentRpcQueryAtomFamily(runtime, {91 const activity = createEnvironmentRpcQueryAtomFamily(runtime, {
92 label: "environment-data:pull-requests:activity",92 label: "environment-data:pull-requests:activity",
93 tag: WS_METHODS.pullRequestsActivity,93 tag: WS_METHODS.pullRequestsActivity,
94 staleTimeMs: 15_000,94 staleTimeMs: 60_000,
95 refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }),95 refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }),
96 });96 });
97 return {97 return {
...@@ -118,7 +118,7 @@ export function createPullRequestEnvironmentAtoms<R, E>(...@@ -118,7 +118,7 @@ export function createPullRequestEnvironmentAtoms<R, E>(
118 detail: createEnvironmentRpcQueryAtomFamily(runtime, {118 detail: createEnvironmentRpcQueryAtomFamily(runtime, {
119 label: "environment-data:pull-requests:detail",119 label: "environment-data:pull-requests:detail",
120 tag: WS_METHODS.pullRequestsDetail,120 tag: WS_METHODS.pullRequestsDetail,
121 staleTimeMs: 15_000,121 staleTimeMs: 60_000,
122 refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }),122 refreshTrigger: ({ environmentId }) => refreshes({ environmentId, input: {} }),
123 }),123 }),
124 activity,124 activity,