From 47a0dd6f5df94c4b12268251e1d28f77edf3e299 Mon Sep 17 00:00:00 2001 From: syn Date: Sat, 18 Jul 2026 00:32:28 -0500 Subject: [PATCH 1/2] Speed up PR review thread loading --- .changeset/quick-review-threads.md | 5 + .plans/FRONTEND_STATIC_REVIEW_20260715.html | 23 ++- .plans/PR_REVIEW_PERF_PLAN.md | 69 +++++++- benchmarks/pr-review-performance.mjs | 116 ++++++++++--- src/github.test.ts | 183 ++++++++++++++++++++ src/modules/github/client.ts | 13 +- src/modules/github/comments.ts | 149 +++++++++++++++- src/modules/github/index.ts | 3 + src/modules/github/schemas.ts | 43 +++++ src/modules/pr-events/service.ts | 47 ++++- src/pr-event-state.test.ts | 19 ++ src/server/routes/github.ts | 2 + web/src/api/github.ts | 7 +- 13 files changed, 627 insertions(+), 52 deletions(-) create mode 100644 .changeset/quick-review-threads.md diff --git a/.changeset/quick-review-threads.md b/.changeset/quick-review-threads.md new file mode 100644 index 00000000..e4ce4a90 --- /dev/null +++ b/.changeset/quick-review-threads.md @@ -0,0 +1,5 @@ +--- +'neondeck': patch +--- + +Load PR review threads through a lean cancellable GitHub query and a short-lived mutation-aware cache so warm review surfaces open within the performance budget. diff --git a/.plans/FRONTEND_STATIC_REVIEW_20260715.html b/.plans/FRONTEND_STATIC_REVIEW_20260715.html index 0298004b..3e70208e 100644 --- a/.plans/FRONTEND_STATIC_REVIEW_20260715.html +++ b/.plans/FRONTEND_STATIC_REVIEW_20260715.html @@ -871,6 +871,9 @@

Executive summary

long tasks. That baseline remains retained. Stable thread identity, local metadata reuse, and active-patch-first scheduling now produce one thread request, zero abandoned patches, and a 798 ms first-patch median that passes all three samples. + The final thread-surface pass keeps the full Flue action intact while using an + 84.7% smaller web query plus a bounded 15-second cache; warm thread visibility now + measures 459 ms median and passes its target. The original results remain retained.

@@ -920,10 +923,10 @@

Correctness risks

Performance risks

- The large-patch and chat render bottlenecks are now mitigated. Remaining risks are - duplicate review-thread reads, repeated full local metadata work for each patch, - Runtime endpoint cost and invalidation bursts, and the eager plugin registry - pending explicit budgets. + The large-patch, chat render, repeated metadata, and warm review-thread + bottlenecks are now mitigated. Remaining risks are the 642 ms tree median, + one-time cold object/thread reads, Runtime endpoint cost and invalidation bursts, + and the eager plugin registry pending explicit budgets.

@@ -993,7 +996,9 @@

Performance first: discussion report

complete. The two approved request-path fixes are now implemented and remeasured: review-thread identity is stable and immutable local PR metadata is reused across patch reads. Active-patch prioritization is also implemented and remeasured: the - selected patch now passes the one-second target in all three retained samples. Keep + selected patch now passes the one-second target in all three retained samples. The + production-only thread pass is complete: a lean cancellable query and short-lived + mutation-aware cache reduce warm thread visibility to 459 ms median. Keep tree and cold-fetch changes, Runtime aggregation, review-subtree consolidation, and registry splitting behind workload and budget evidence. @@ -1022,6 +1027,8 @@

Performance first: discussion report

Real PR · request stability1 thread request · 0 aborted patches381 kB thread · 743 kB total APILast API response: 2,782 msDuplicate and settlement churn eliminated Real PR · active-patch priority612 ms tree · 798 ms first patch620 ms LCP · 0.0012 CLS1,724 ms threads · zero long tasksCompleted · first patch passes all samples Real PR · prioritized requests1 thread request · 4 patch requests381 kB thread · 724 kB total API0 aborted · last API response 2,021 msActive read settles before background fanout + Real PR · lean threads, uncached639 ms tree · 924 ms first patch58 kB thread · 401 kB total API1,511 ms threads · 16.8 ms post-response renderPayload fixed; GitHub latency remains + Real PR · lean threads, warm642 ms tree · 934 ms first patch530 ms traced LCP · 0.00 CLS459 ms threads · 6.8 ms backend cache hitCompleted · warm thread target passes Real PR · cold local / GitHub4,978 / 3,894 ms295 / 765 kB metadata0 / 1 truncated filesBoth miss cold target; local is complete Production initial preload194.6 kB gzip157.7 kB entry · 21.6 kB ccountFocused-review wrapper 0.77 kB gzipTrack; no budget miss declared @@ -1047,7 +1054,10 @@

Performance first: discussion report

measured 551 ms LCP, 0.00 CLS, and the same 39 ms total Pierre forced reflow with no estimated savings. After active-patch prioritization, the browser median reached 798 ms to first patch and a direct trace measured 498 ms LCP, 0.00 CLS, and 36 ms total - Pierre forced reflow with no estimated savings. + Pierre forced reflow with no estimated savings. The final production-only thread + pass measured 459 ms median thread visibility, 58 kB per thread response, 6.5 ms + post-response rendering, zero aborted/failed API requests, 530 ms traced LCP, and + 0.00 CLS. Its initial uncached GitHub read remains outside the warm target.
@@ -1352,6 +1362,7 @@

Recommended sequence

  • COMPLETED · MEASUREMENT + FIRST WAVE Performance measurement gate: the five-part fixture pass, chat isolation, thresholded Pierre CodeView, and real registered-PR trace are complete. Runtime topology, duplicate review DOM, and lazy registry remain deferred behind workload/budget evidence.
  • COMPLETED · REQUEST-PATH REMEDIATION P1 · Real PR request path: discuss the measured duplicate review-thread identity and repeated local metadata work, then implement only the approved fixes and repeat npm run bench:pr-review. The stable thread key and bounded revision-metadata cache are implemented and verified on the retained target.
  • COMPLETED · ACTIVE PATCH PRIORITY P1 · Active patch priority: discuss whether neighbor and unresolved-path prefetch should wait for the active patch. The retained remediated trace was 195 ms over target; the implemented active-first schedule now reaches a 798 ms median and passes all three samples while preserving zero abandoned reads.
    • +
  • COMPLETED · WARM THREAD PATH P1 · Review-thread latency: correct the harness's production-mode detection, measure request versus render cost, slim the web-only GitHub query, and add only a bounded 15-second mutation-aware cache after the lean query remained too slow. Warm thread visibility now reaches 459 ms median with an 84.7% smaller response; cold GitHub latency remains explicit.
  • COMPLETED · COPY/DISCLOSURE P2 · Copy/disclosure: row-specific action names and full-value affordances for operational text. Suggested command: $impeccable clarify.
  • COMPLETED · PR #140 P2 · Editor state ownership: keep new-comment, draft-edit, and thread-reply bodies scoped to their editor generation so older async completions cannot clear newer work.
  • COMPLETED · FINAL PASS P3 · Final pass: tabular metrics, timer cleanup, operation-scoped status, stable finding IDs, and config tightening. Suggested command: $impeccable polish.
    • diff --git a/.plans/PR_REVIEW_PERF_PLAN.md b/.plans/PR_REVIEW_PERF_PLAN.md index cf89395a..79f02c1f 100644 --- a/.plans/PR_REVIEW_PERF_PLAN.md +++ b/.plans/PR_REVIEW_PERF_PLAN.md @@ -1,6 +1,6 @@ # PR Review / File Tree Performance Plan -Status: phases 1–5 implemented; real-PR verification, request-path remediation, and active-patch prioritization complete; cold-fetch decision remains +Status: phases 1–5 implemented; real-PR verification, request-path remediation, active-patch prioritization, and warm review-thread remediation complete; tree and cold-fetch decisions remain Prior art: `.plans/archived/DIFF_UI_PLAN.md`, `.plans/archived/DIFF_REVIEW.md` ## 2026-07-17 reconciliation @@ -266,6 +266,59 @@ but before the custom element painted. A direct DevTools trace measured 498 ms LCP, 0.00 CLS, and a 36 ms Pierre forced-reflow total with no estimated user-visible savings. +## 2026-07-18 review-thread surface result + +A production-only follow-up isolated the remaining thread delay. The previous +1,724 ms result is retained above, but the rerun found that its browser origin +included the Vite development client despite being described as production. +React Strict Mode could start and abort an initial request in that mode. The +benchmark now detects Vite development mode and refuses it unless +`--allow-development` is explicit. It also records failed and aborted API +requests plus thread request start, response end, duration, bytes, and +post-response render time. + +The clean production baseline still missed: one 381 KB thread response took +about 810 ms directly and thread visibility measured 1,644 ms median. Three +instrumented reloads showed a roughly 362 ms request start, 1,011 ms request +duration, and only 11 ms of React work after the response. This confirmed the +GitHub GraphQL path, not TanStack Query, React, or Pierre rendering, as the +bottleneck. + +The review surface now uses a dedicated GitHub query that omits diff hunks, +review/database ids, and pull-request backreferences that the UI does not read. +The full Flue action continues using the full-fidelity query. The HTTP response +also omits duplicate unresolved-thread and unresolved-comment collections; the +client derives unresolved threads from the canonical list. The lean uncached +run reduced thread transfer from 381,244 B to 58,240 B, but still measured +1,511 ms thread visibility because the GitHub request remained 906–1,119 ms. + +Because the lean query could not reach the warm budget alone, it is backed by a +small in-process cache: 15-second TTL, 16 entries, token-scoped keys, and +explicit invalidation after review submission, thread reply, and +resolve/unresolve. Reads invalidated while in flight are not stored. In-flight +requests are not shared, so one browser cancellation cannot cancel another +caller's work. Browser cancellation is propagated through the GraphQL request +to GitHub. + +| Path | Active priority | Lean, no cache | Lean + warm cache | Change / verdict | +| ------------------------------ | --------------: | -------------: | ----------------: | ------------------------------------- | +| Production tree visible | 612 ms | 639 ms | 642 ms | Still misses; separate tree follow-up | +| Production first patch visible | 798 ms | 924 ms | 934 ms | Median passes | +| Production threads visible | 1,724 ms | 1,511 ms | 459 ms | 73.4% faster; median passes | +| Initial backend thread read | 917 ms | 684 ms | 655 ms | Cold GitHub round trip remains | +| Warm backend thread read | not recorded | not applicable | 6.8 ms | Short-lived in-process reuse | +| Thread transfer per sample | 381,244 B | 58,240 B | 58,240 B | 84.7% smaller | +| Total API transfer per sample | 742,850 B | 400,821 B | 400,821 B | 46.0% smaller | +| Thread render after response | not recorded | 16.8 ms | 6.5 ms | Rendering remains negligible | +| Aborted or failed API requests | 0 | 0 | 0 | Stable production path | + +The final three thread samples were 608, 452, and 459 ms. The first sample's +604 ms FCP made a sub-500 ms thread paint impossible even though its cached +thread request took 23 ms; the median satisfies the warm harness target. A +separate final DevTools trace measured 530 ms LCP, 0.00 CLS, one successful +thread request, and zero estimated FCP/LCP savings from render-blocking +resources. The cold first read remains explicitly outside the warm-cache pass. + ## Confirmed follow-up candidates and remediation status 1. **Completed — stabilize review-thread identity.** `reviewThreads(pr)` @@ -283,15 +336,21 @@ user-visible savings. isolated from adjacent, draft, and unresolved background reads until it settles. The next run reached a 798 ms median and passed the target in all three samples without abandoned reads. -4. **Discuss later — revisit cold fetch.** The 4.98-second object fetch misses the target, +4. **Completed — slim and briefly reuse review-surface threads.** The web path + now uses an 84.7% smaller query response plus a bounded 15-second cache with + mutation invalidation and race protection. Production thread visibility is + 459 ms median, while the full-fidelity Flue action is unchanged. +5. **Discuss later — revisit cold fetch.** The 4.98-second object fetch misses the target, but it is a one-time revision cost. Separate network fetch time from local metadata time before changing refspecs or the `<3s` budget. Acceptance is partial on the same real target: duplicate thread requests and settlement-driven abandoned patch reads are eliminated, the first-patch browser -budget now passes, backend targets pass, and fallback code is unchanged. The -tree, threads, and one-time cold-object budgets still miss and remain separate -follow-ups. Raw baseline, remediation, and active-priority results are +budget now passes, warm thread visibility passes on the median, backend targets +pass, and fallback code is unchanged. The tree and one-time cold-object budgets +still miss and remain separate follow-ups; a cold GitHub thread read also +remains slower than the warm UI budget. Raw baseline, remediation, and +active-priority results are gitignored at `benchmarks/results/pr-12204-real-local.json` and `benchmarks/results/pr-12204-remediation-local.json`, and diff --git a/benchmarks/pr-review-performance.mjs b/benchmarks/pr-review-performance.mjs index d7b10993..7b628399 100644 --- a/benchmarks/pr-review-performance.mjs +++ b/benchmarks/pr-review-performance.mjs @@ -14,6 +14,7 @@ const baseRef = readStringArg('--base-ref', 'main'); const title = readStringArg('--title', `${repo}#${number}`); const samples = readPositiveIntegerArg('--samples', 3); const includeGitHubFallback = args.includes('--include-github-fallback'); +const allowDevelopment = args.includes('--allow-development'); const outPath = readStringArg( '--out', 'benchmarks/results/pr-review-real-local.json', @@ -21,7 +22,12 @@ const outPath = readStringArg( const [owner, name] = parseRepo(repo); const target = { repo, number, head, base, baseRef, title }; -await assertServerAvailable(origin); +const serverMode = await inspectServer(origin); +if (serverMode === 'development' && !allowDevelopment) { + throw new Error( + `${origin} is serving the Vite development client. Run npm run build:dashboard followed by npm start, or pass --allow-development for a diagnostic run.`, + ); +} const initialAuto = await measureJson(fileListUrl('auto')); const files = initialAuto.json.data?.files ?? []; @@ -41,13 +47,27 @@ const activeIndex = files.findIndex((file) => file.path === activeFile.path); const warmFirstPatch = await repeat(samples, () => measureJson(fileDiffUrl(activeFile.path, 'auto')), ); -const threads = await measureJson(`${origin}/api/github/prs/review-threads`, { - method: 'POST', - headers: { 'content-type': 'application/json' }, - body: JSON.stringify({ repo, prNumber: number }), -}); -const threadData = threads.json.data ?? {}; -const unresolvedPaths = (threadData.unresolvedReviewThreads ?? []) +const initialThreads = await measureJson( + `${origin}/api/github/prs/review-threads`, + { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ repo, prNumber: number }), + }, +); +const warmReviewThreads = await repeat(samples, () => + measureJson(`${origin}/api/github/prs/review-threads`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ repo, prNumber: number }), + }), +); +const threadData = initialThreads.json.data ?? {}; +const reviewThreads = threadData.reviewThreads ?? []; +const unresolvedReviewThreads = + threadData.unresolvedReviewThreads ?? + reviewThreads.filter((thread) => !thread.isResolved); +const unresolvedPaths = unresolvedReviewThreads .map((thread) => thread.path ?? thread.comments?.[0]?.path) .filter(Boolean); const fanoutPaths = [ @@ -83,7 +103,7 @@ try { measureBrowserReview(browser, { origin, target, - threadCount: threadData.reviewThreads?.length ?? 0, + threadCount: reviewThreads.length, }), ); } finally { @@ -91,7 +111,7 @@ try { } const output = { - version: 2, + version: 3, generatedAt: new Date().toISOString(), environment: { platform: process.platform, @@ -99,6 +119,7 @@ const output = { node: process.version, browser: 'Playwright Chromium', origin, + serverMode, samples, note: 'Machine-local real-PR evidence; not a CI performance gate.', }, @@ -107,8 +128,8 @@ const output = { fileCount: files.length, additions: initialAuto.json.data?.diffSummary?.additions ?? null, deletions: initialAuto.json.data?.diffSummary?.deletions ?? null, - reviewThreads: threadData.reviewThreads?.length ?? 0, - unresolvedReviewThreads: threadData.unresolvedReviewThreads?.length ?? 0, + reviewThreads: reviewThreads.length, + unresolvedReviewThreads: unresolvedReviewThreads.length, }, budgets: { warmTreeVisibleMs: 500, @@ -128,9 +149,14 @@ const output = { ...aggregateRequests(warmFirstPatch), }, reviewThreads: { - ...requestSummary(threads), - total: threadData.reviewThreads?.length ?? 0, - unresolved: threadData.unresolvedReviewThreads?.length ?? 0, + initial: { + ...requestSummary(initialThreads), + warning: + 'This is cold only when the in-process review-thread cache was empty before the run.', + }, + warm: aggregateRequests(warmReviewThreads), + total: reviewThreads.length, + unresolved: unresolvedReviewThreads.length, unresolvedPaths: [...new Set(unresolvedPaths)], }, concurrentFanout, @@ -208,6 +234,14 @@ async function measureBrowserReview( colorScheme: 'dark', }); const page = await context.newPage(); + const failedApiRequests = []; + page.on('requestfailed', (request) => { + if (!request.url().includes('/api/github/')) return; + failedApiRequests.push({ + url: request.url(), + errorText: request.failure()?.errorText ?? 'unknown', + }); + }); const url = new URL('/review', browserOrigin); url.searchParams.set('repo', browserTarget.repo); url.searchParams.set('number', String(browserTarget.number)); @@ -296,7 +330,7 @@ async function measureBrowserReview( { timeout: 15_000 }, ); await page.waitForTimeout(3_000); - return await page.evaluate(() => { + const browserResult = await page.evaluate(() => { const state = window.__NEONDECK_REAL_PR_PERF__; const resources = performance .getEntriesByType('resource') @@ -310,6 +344,10 @@ async function measureBrowserReview( const firstContentfulPaint = performance.getEntriesByName( 'first-contentful-paint', )[0]; + const threadResponseEndMs = Math.max( + 0, + ...threadRequests.map((entry) => entry.responseEnd), + ); return { treeVisibleMs: state.treeVisibleMs, firstPatchMs: state.firstPatchMs, @@ -332,19 +370,47 @@ async function measureBrowserReview( (sum, entry) => sum + entry.transferSize, 0, ), + threadEncodedBodyBytes: threadRequests.reduce( + (sum, entry) => sum + entry.encodedBodySize, + 0, + ), + threadRequestStartMs: + threadRequests.length > 0 + ? Math.min(...threadRequests.map((entry) => entry.startTime)) + : 0, + threadResponseEndMs, + threadRequestDurationMs: Math.max( + 0, + ...threadRequests.map((entry) => entry.duration), + ), + threadRenderAfterResponseMs: + threadResponseEndMs > 0 + ? Math.max(0, state.threadsVisibleMs - threadResponseEndMs) + : 0, patchRequestCount: patchRequests.length, patchRequestsStartedBeforeFirstPatch: patchRequests.filter( (entry) => entry.startTime < state.firstPatchMs, ).length, - abortedPatchRequestCount: patchRequests.filter( - (entry) => entry.transferSize === 0, - ).length, lastApiResponseMs: Math.max( 0, ...resources.map((entry) => entry.responseEnd), ), }; }); + const abortedRequests = failedApiRequests.filter((request) => + request.errorText.includes('ERR_ABORTED'), + ); + return { + ...browserResult, + failedApiRequestCount: failedApiRequests.length, + abortedApiRequestCount: abortedRequests.length, + abortedThreadRequestCount: abortedRequests.filter((request) => + request.url.endsWith('/review-threads'), + ).length, + abortedPatchRequestCount: abortedRequests.filter((request) => + request.url.includes('/files/diff?'), + ).length, + }; } finally { await context.close(); } @@ -379,8 +445,16 @@ function aggregateBrowserSamples(browserResults) { 'apiTransferBytes', 'threadRequestCount', 'threadTransferBytes', + 'threadEncodedBodyBytes', + 'threadRequestStartMs', + 'threadResponseEndMs', + 'threadRequestDurationMs', + 'threadRenderAfterResponseMs', 'patchRequestCount', 'patchRequestsStartedBeforeFirstPatch', + 'failedApiRequestCount', + 'abortedApiRequestCount', + 'abortedThreadRequestCount', 'abortedPatchRequestCount', 'lastApiResponseMs', ].map((key) => [ @@ -423,11 +497,13 @@ async function repeat(count, fn) { return values; } -async function assertServerAvailable(serverOrigin) { +async function inspectServer(serverOrigin) { const response = await fetch(serverOrigin); if (!response.ok) { throw new Error(`${serverOrigin} returned ${response.status}.`); } + const html = await response.text(); + return html.includes('/@vite/client') ? 'development' : 'production'; } function parseRepo(value) { diff --git a/src/github.test.ts b/src/github.test.ts index 208786a4..a997598e 100644 --- a/src/github.test.ts +++ b/src/github.test.ts @@ -6,14 +6,17 @@ import { addPrReviewDraftComment, buildPullRequestQueries, clearGitHubPullRequestQueueCache, + clearPullRequestReviewSurfaceThreadCache, deletePrReviewNeonSeedsForComments, fetchFailingCheckFacts, fetchGitHubIssues, fetchCheckSummary, fetchPullRequestFiles, + fetchPullRequestReviewSurfaceThreadsWithMetadata, fetchPullRequestReviewThreads, fetchPullRequestReviewThreadsWithMetadata, fetchPullRequestQueue, + invalidatePullRequestReviewSurfaceThreadCache, postPullRequestComment, readLivePrReviewDraft, recordPrReviewNeonSeed, @@ -35,6 +38,7 @@ const tempRoots: string[] = []; afterEach(async () => { globalThis.fetch = originalFetch; clearGitHubPullRequestQueueCache(); + clearPullRequestReviewSurfaceThreadCache(); vi.restoreAllMocks(); await Promise.all( tempRoots @@ -786,6 +790,185 @@ describe('github foundation', () => { }); }); + it('uses a lean review-thread query for the interactive review surface', async () => { + const fetchedBodies: Array<{ query?: string }> = []; + const controller = new AbortController(); + const requestSignals: AbortSignal[] = []; + globalThis.fetch = vi.fn(async (_input, init) => { + if (init?.signal) requestSignals.push(init.signal); + fetchedBodies.push(JSON.parse(String(init?.body ?? '{}'))); + return jsonResponse({ + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ + { + id: 'thread-1', + isResolved: false, + isOutdated: false, + path: 'src/app.ts', + line: 12, + originalLine: null, + diffSide: 'RIGHT', + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ + { + id: 'comment-1', + body: 'Surface comment', + url: 'https://example.test/comment-1', + author: { login: 'reviewer' }, + createdAt: '2026-06-30T20:05:00Z', + updatedAt: '2026-06-30T20:05:00Z', + path: 'src/app.ts', + line: 12, + originalLine: null, + }, + ], + }, + }, + ], + }, + }, + }, + }, + }); + }); + + await expect( + fetchPullRequestReviewSurfaceThreadsWithMetadata({ + token: 'token', + owner: 'pandemicsyn', + repo: 'neondeck', + number: 123, + signal: controller.signal, + }), + ).resolves.toMatchObject({ + reviewThreads: [ + { + id: 'thread-1', + pullRequestRepo: null, + pullRequestNumber: null, + comments: [ + { + id: 'comment-1', + databaseId: null, + diffHunk: null, + reviewId: null, + }, + ], + }, + ], + truncated: false, + }); + controller.abort(); + expect(requestSignals[0]?.aborted).toBe(true); + await fetchPullRequestReviewSurfaceThreadsWithMetadata({ + token: 'token', + owner: 'pandemicsyn', + repo: 'neondeck', + number: 123, + }); + expect(globalThis.fetch).toHaveBeenCalledTimes(1); + invalidatePullRequestReviewSurfaceThreadCache({ + owner: 'pandemicsyn', + repo: 'neondeck', + number: 123, + }); + await fetchPullRequestReviewSurfaceThreadsWithMetadata({ + token: 'token', + owner: 'pandemicsyn', + repo: 'neondeck', + number: 123, + }); + expect(globalThis.fetch).toHaveBeenCalledTimes(2); + expect(fetchedBodies[0]?.query).toContain( + 'NeondeckPullRequestReviewSurfaceThreads', + ); + expect(fetchedBodies[0]?.query).not.toContain('diffHunk'); + expect(fetchedBodies[0]?.query).not.toContain('pullRequestReview'); + expect(fetchedBodies[0]?.query).not.toContain('databaseId'); + }); + + it('does not cache a review-thread read invalidated while in flight', async () => { + const body = { + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [], + }, + }, + }, + }, + }; + let resolveFirst!: (response: Response) => void; + let calls = 0; + globalThis.fetch = vi.fn(async () => { + calls += 1; + if (calls === 1) { + return new Promise((resolve) => { + resolveFirst = resolve; + }); + } + return jsonResponse(body); + }); + + const first = fetchPullRequestReviewSurfaceThreadsWithMetadata({ + token: 'token', + owner: 'pandemicsyn', + repo: 'neondeck', + number: 123, + }); + await vi.waitFor(() => expect(globalThis.fetch).toHaveBeenCalledTimes(1)); + invalidatePullRequestReviewSurfaceThreadCache({ + owner: 'pandemicsyn', + repo: 'neondeck', + number: 123, + }); + resolveFirst(jsonResponse(body)); + await first; + await fetchPullRequestReviewSurfaceThreadsWithMetadata({ + token: 'token', + owner: 'pandemicsyn', + repo: 'neondeck', + number: 123, + }); + + expect(globalThis.fetch).toHaveBeenCalledTimes(2); + }); + + it('preserves caller cancellation instead of reporting a GitHub timeout', async () => { + globalThis.fetch = vi.fn(async (_input, init) => { + const signal = init?.signal; + if (!signal) throw new Error('Expected a request signal.'); + return new Promise((_resolve, reject) => { + if (signal.aborted) { + reject(signal.reason); + return; + } + signal.addEventListener('abort', () => reject(signal.reason), { + once: true, + }); + }); + }); + const controller = new AbortController(); + const request = fetchPullRequestReviewSurfaceThreadsWithMetadata({ + token: 'token', + owner: 'pandemicsyn', + repo: 'neondeck', + number: 124, + signal: controller.signal, + }); + + controller.abort(); + + await expect(request).rejects.toMatchObject({ name: 'AbortError' }); + }); + it('logs when review thread pagination reaches the page cap', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); let page = 0; diff --git a/src/modules/github/client.ts b/src/modules/github/client.ts index 84bf6035..aecc839c 100644 --- a/src/modules/github/client.ts +++ b/src/modules/github/client.ts @@ -22,10 +22,12 @@ export async function githubGraphqlFetch( token: string, query: string, variables: Record, + options: { signal?: AbortSignal } = {}, ) { const response = await githubFetch(token, 'https://api.github.com/graphql', { method: 'POST', body: JSON.stringify({ query, variables }), + signal: options.signal, }); const data = await response.json(); const parsed = v.parse(githubGraphqlBaseResponseSchema, data); @@ -44,6 +46,7 @@ export async function githubFetch( init: RequestInit = {}, ) { let response: Response; + const timeoutSignal = AbortSignal.timeout(githubRequestTimeoutMs); try { response = await fetch(url, { ...init, @@ -55,10 +58,16 @@ export async function githubFetch( 'User-Agent': 'neondeck', 'X-GitHub-Api-Version': '2022-11-28', }, - signal: AbortSignal.timeout(githubRequestTimeoutMs), + signal: init.signal + ? AbortSignal.any([init.signal, timeoutSignal]) + : timeoutSignal, }); } catch (error) { - if (isRequestTimeout(error)) { + if ( + timeoutSignal.aborted && + !init.signal?.aborted && + isRequestTimeout(error) + ) { throw new Error( `GitHub request timed out after ${Math.round(githubRequestTimeoutMs / 1000)}s`, ); diff --git a/src/modules/github/comments.ts b/src/modules/github/comments.ts index 81bb5b4a..30c44300 100644 --- a/src/modules/github/comments.ts +++ b/src/modules/github/comments.ts @@ -1,3 +1,4 @@ +import { createHash } from 'node:crypto'; import * as v from 'valibot'; import { encodePathSegment, githubFetch, githubGraphqlFetch } from './client'; import { @@ -6,6 +7,7 @@ import { githubReviewThreadCommentsGraphqlResponseSchema, githubReviewThreadsGraphqlResponseSchema, pullRequestReviewThreadNodeQuery, + pullRequestReviewSurfaceThreadsQuery, pullRequestReviewThreadsQuery, reviewThreadCommentsQuery, } from './schemas'; @@ -17,6 +19,22 @@ import type { GitHubReviewThreadGraphqlNode, } from './schemas'; +const reviewSurfaceCacheTtlMs = 15_000; +const reviewSurfaceCacheMaxEntries = 16; +const reviewSurfaceCache = new Map(); +const reviewSurfaceTargetEpochs = new Map(); + +type ReviewThreadsWithMetadata = { + reviewThreads: GitHubPullRequestReviewThread[]; + truncated: boolean; +}; + +type CachedReviewSurfaceThreads = { + targetKey: string; + expiresAt: number; + value: ReviewThreadsWithMetadata; +}; + export async function postPullRequestComment(options: { token: string; owner: string; @@ -53,6 +71,7 @@ export async function fetchPullRequestReviewThreads(options: { owner: string; repo: string; number: number; + signal?: AbortSignal; }): Promise { return (await fetchPullRequestReviewThreadsWithMetadata(options)) .reviewThreads; @@ -63,9 +82,111 @@ export async function fetchPullRequestReviewThreadsWithMetadata(options: { owner: string; repo: string; number: number; + signal?: AbortSignal; }): Promise<{ reviewThreads: GitHubPullRequestReviewThread[]; truncated: boolean; +}> { + return fetchReviewThreadsWithQuery(options, pullRequestReviewThreadsQuery); +} + +export async function fetchPullRequestReviewSurfaceThreadsWithMetadata(options: { + token: string; + owner: string; + repo: string; + number: number; + signal?: AbortSignal; +}): Promise { + const targetKey = reviewSurfaceTargetKey(options); + const cacheKey = `${targetKey}\u0000${tokenFingerprint(options.token)}`; + const cached = reviewSurfaceCache.get(cacheKey); + if (cached && cached.expiresAt > Date.now()) { + reviewSurfaceCache.delete(cacheKey); + reviewSurfaceCache.set(cacheKey, cached); + return cached.value; + } + if (cached) reviewSurfaceCache.delete(cacheKey); + + const targetEpoch = reviewSurfaceTargetEpochs.get(targetKey) ?? 0; + const value = await fetchReviewThreadsWithQuery( + options, + pullRequestReviewSurfaceThreadsQuery, + ); + if ((reviewSurfaceTargetEpochs.get(targetKey) ?? 0) === targetEpoch) { + storeReviewSurfaceThreads(cacheKey, { + targetKey, + expiresAt: Date.now() + reviewSurfaceCacheTtlMs, + value, + }); + } + return value; +} + +export function invalidatePullRequestReviewSurfaceThreadCache(options: { + owner: string; + repo: string; + number: number; +}) { + const targetKey = reviewSurfaceTargetKey(options); + reviewSurfaceTargetEpochs.set( + targetKey, + (reviewSurfaceTargetEpochs.get(targetKey) ?? 0) + 1, + ); + for (const [key, cached] of reviewSurfaceCache) { + if (cached.targetKey === targetKey) reviewSurfaceCache.delete(key); + } +} + +export function clearPullRequestReviewSurfaceThreadCache() { + reviewSurfaceCache.clear(); + reviewSurfaceTargetEpochs.clear(); +} + +function storeReviewSurfaceThreads( + key: string, + value: CachedReviewSurfaceThreads, +) { + const now = Date.now(); + for (const [cachedKey, cached] of reviewSurfaceCache) { + if (cached.expiresAt <= now) reviewSurfaceCache.delete(cachedKey); + } + reviewSurfaceCache.delete(key); + reviewSurfaceCache.set(key, value); + while (reviewSurfaceCache.size > reviewSurfaceCacheMaxEntries) { + const oldestKey = reviewSurfaceCache.keys().next().value; + if (oldestKey === undefined) break; + reviewSurfaceCache.delete(oldestKey); + } +} + +function reviewSurfaceTargetKey(options: { + owner: string; + repo: string; + number: number; +}) { + return [ + options.owner.toLowerCase(), + options.repo.toLowerCase(), + options.number, + ].join('\u0000'); +} + +function tokenFingerprint(token: string) { + return createHash('sha256').update(token).digest('base64url').slice(0, 16); +} + +async function fetchReviewThreadsWithQuery( + options: { + token: string; + owner: string; + repo: string; + number: number; + signal?: AbortSignal; + }, + query: string, +): Promise<{ + reviewThreads: GitHubPullRequestReviewThread[]; + truncated: boolean; }> { const threads: GitHubPullRequestReviewThread[] = []; let cursor: string | null = null; @@ -74,19 +195,22 @@ export async function fetchPullRequestReviewThreadsWithMetadata(options: { for (let page = 0; page < 5; page += 1) { const data = await githubGraphqlFetch( options.token, - pullRequestReviewThreadsQuery, + query, { owner: options.owner, name: options.repo, number: options.number, after: cursor, }, + { signal: options.signal }, ); const parsed = v.parse(githubReviewThreadsGraphqlResponseSchema, data); const pullRequest = parsed.data.repository?.pullRequest; if (!pullRequest) break; for (const thread of pullRequest.reviewThreads.nodes ?? []) { - threads.push(await normalizeReviewThread(options.token, thread)); + threads.push( + await normalizeReviewThread(options.token, thread, options.signal), + ); } if (!pullRequest.reviewThreads.pageInfo.hasNextPage) break; @@ -110,11 +234,13 @@ export async function fetchPullRequestReviewThreadsWithMetadata(options: { export async function fetchPullRequestReviewThread(options: { token: string; threadId: string; + signal?: AbortSignal; }): Promise { const data = await githubGraphqlFetch( options.token, pullRequestReviewThreadNodeQuery, { threadId: options.threadId }, + { signal: options.signal }, ); const parsed = v.parse(githubReviewThreadNodeGraphqlResponseSchema, data); const thread = parsed.data.node; @@ -123,14 +249,15 @@ export async function fetchPullRequestReviewThread(options: { `GitHub review thread "${options.threadId}" was not found.`, ); } - return normalizeReviewThread(options.token, thread); + return normalizeReviewThread(options.token, thread, options.signal); } async function normalizeReviewThread( token: string, thread: GitHubReviewThreadGraphqlNode, + signal?: AbortSignal, ): Promise { - const comments = await fetchAllReviewThreadComments(token, thread); + const comments = await fetchAllReviewThreadComments(token, thread, signal); return { id: thread.id, isResolved: thread.isResolved, @@ -151,6 +278,7 @@ async function normalizeReviewThread( async function fetchAllReviewThreadComments( token: string, thread: GitHubReviewThreadGraphqlNode, + signal?: AbortSignal, ) { const comments = [...(thread.comments.nodes ?? [])]; let cursor = thread.comments.pageInfo.endCursor; @@ -163,10 +291,15 @@ async function fetchAllReviewThreadComments( truncated = true; break; } - const data = await githubGraphqlFetch(token, reviewThreadCommentsQuery, { - threadId: thread.id, - after: cursor, - }); + const data = await githubGraphqlFetch( + token, + reviewThreadCommentsQuery, + { + threadId: thread.id, + after: cursor, + }, + { signal }, + ); const parsed = v.parse( githubReviewThreadCommentsGraphqlResponseSchema, data, diff --git a/src/modules/github/index.ts b/src/modules/github/index.ts index bfa8fa54..1d1bd346 100644 --- a/src/modules/github/index.ts +++ b/src/modules/github/index.ts @@ -63,10 +63,13 @@ export type { GitHubPrReviewVerdict, } from './reviews'; export { + clearPullRequestReviewSurfaceThreadCache, fetchPullRequestReviewThread, + fetchPullRequestReviewSurfaceThreadsWithMetadata, fetchPullRequestReviewThreads, postPullRequestComment, fetchPullRequestReviewThreadsWithMetadata, + invalidatePullRequestReviewSurfaceThreadCache, } from './comments'; export type { GitHubBranchPushPermissions, diff --git a/src/modules/github/schemas.ts b/src/modules/github/schemas.ts index e016fb56..8fa2e6da 100644 --- a/src/modules/github/schemas.ts +++ b/src/modules/github/schemas.ts @@ -712,6 +712,49 @@ export const pullRequestReviewThreadsQuery = ` } `; +export const pullRequestReviewSurfaceThreadsQuery = ` + query NeondeckPullRequestReviewSurfaceThreads($owner: String!, $name: String!, $number: Int!, $after: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $after) { + pageInfo { + hasNextPage + endCursor + } + nodes { + id + isResolved + isOutdated + path + line + originalLine + diffSide + comments(first: 100) { + pageInfo { + hasNextPage + endCursor + } + nodes { + id + body + url + author { + login + } + createdAt + updatedAt + path + line + originalLine + } + } + } + } + } + } + } +`; + export const reviewThreadCommentsQuery = ` query NeondeckPullRequestReviewThreadComments($threadId: ID!, $after: String) { node(id: $threadId) { diff --git a/src/modules/pr-events/service.ts b/src/modules/pr-events/service.ts index cd795d18..6f485114 100644 --- a/src/modules/pr-events/service.ts +++ b/src/modules/pr-events/service.ts @@ -9,9 +9,11 @@ import { fetchPullRequestEventState, fetchPullRequestFiles, fetchPullRequestFilesWithCache, + fetchPullRequestReviewSurfaceThreadsWithMetadata, fetchPullRequestReviewThreadsWithMetadata, fetchPullRequestReviewThread, GitHubPrReviewSubmitError, + invalidatePullRequestReviewSurfaceThreadCache, postPullRequestComment, pullRequestEventStateTruncation, readLivePrReviewDraft, @@ -109,6 +111,10 @@ export async function getGitHubPrReviewThreads( input: v.InferInput, paths: RuntimePaths = runtimePaths(), dependencies: PrEventStateDependencies = {}, + options: { + signal?: AbortSignal; + surface?: boolean; + } = {}, ): Promise { const action = 'github_pr_review_threads_get'; await ensureRuntimeHome(paths); @@ -134,12 +140,15 @@ export async function getGitHubPrReviewThreads( try { const fetcher = dependencies.fetchPullRequestReviewThreads ?? - fetchPullRequestReviewThreadsWithMetadata; + (options.surface + ? fetchPullRequestReviewSurfaceThreadsWithMetadata + : fetchPullRequestReviewThreadsWithMetadata); const result = await fetcher({ token, owner: resolved.target.owner, repo: resolved.target.repo, number: resolved.target.number, + signal: options.signal, }); threads = result.reviewThreads; truncated = @@ -165,14 +174,19 @@ export async function getGitHubPrReviewThreads( action, false, `Fetched ${threads.length} review thread(s) for ${resolved.target.repoFullName}#${resolved.target.number}.`, - { - target: eventTargetJson(resolved.target), - reviewThreads: threads as unknown as JsonValue, - reviewThreadsTruncated: truncated, - unresolvedReviewThreads: unresolvedThreads as unknown as JsonValue, - unresolvedReviewComments: - unresolvedReviewComments as unknown as JsonValue, - }, + options.surface + ? { + reviewThreads: threads as unknown as JsonValue, + reviewThreadsTruncated: truncated, + } + : { + target: eventTargetJson(resolved.target), + reviewThreads: threads as unknown as JsonValue, + reviewThreadsTruncated: truncated, + unresolvedReviewThreads: unresolvedThreads as unknown as JsonValue, + unresolvedReviewComments: + unresolvedReviewComments as unknown as JsonValue, + }, ); } @@ -860,6 +874,11 @@ export async function postGitHubPrReview( commentIds: parsedReview.output.commentIds, fetchHeadSha: dependencies.fetchPullRequestHeadSha, }); + invalidatePullRequestReviewSurfaceThreadCache({ + owner: resolved.target.owner, + repo: resolved.target.repo, + number: resolved.target.number, + }); return okResult( 'github_pr_review_post', true, @@ -1030,6 +1049,11 @@ export async function postGitHubPrThreadReply( threadId, body: parsed.output.text, }); + invalidatePullRequestReviewSurfaceThreadCache({ + owner: resolved.target.owner, + repo: resolved.target.repo, + number: resolved.target.number, + }); return okResult(action, true, 'Posted review thread reply.', { thread: thread as unknown as JsonValue, }); @@ -1104,6 +1128,11 @@ export async function postGitHubPrThreadResolution( : (dependencies.unresolvePullRequestReviewThread ?? unresolvePullRequestReviewThread); const thread = await mutator({ token, threadId }); + invalidatePullRequestReviewSurfaceThreadCache({ + owner: target.target.owner, + repo: target.target.repo, + number: target.target.number, + }); return okResult( action, true, diff --git a/src/pr-event-state.test.ts b/src/pr-event-state.test.ts index c095f092..63283cb2 100644 --- a/src/pr-event-state.test.ts +++ b/src/pr-event-state.test.ts @@ -319,6 +319,25 @@ describe('PR event state watermarks', () => { ], }, }); + const surfaceResult = await getGitHubPrReviewThreads( + { repo: 'neondeck', prNumber: 123 }, + paths, + dependencies, + { surface: true }, + ); + expect(surfaceResult).toMatchObject({ + ok: true, + data: { + reviewThreads: [ + expect.objectContaining({ id: 'thread-1' }), + expect.objectContaining({ id: 'thread-2' }), + ], + reviewThreadsTruncated: false, + }, + }); + expect(surfaceResult.data).not.toHaveProperty('target'); + expect(surfaceResult.data).not.toHaveProperty('unresolvedReviewThreads'); + expect(surfaceResult.data).not.toHaveProperty('unresolvedReviewComments'); await expect( getGitHubPrRequestedChanges( { repo: 'neondeck', prNumber: 123 }, diff --git a/src/server/routes/github.ts b/src/server/routes/github.ts index 54f039af..0e6c2bf2 100644 --- a/src/server/routes/github.ts +++ b/src/server/routes/github.ts @@ -478,6 +478,8 @@ export function createGitHubRoutes(paths: RuntimePaths) { const result = await getGitHubPrReviewThreads( (await safeJsonBody(c)) as Parameters[0], paths, + {}, + { signal: c.req.raw.signal, surface: true }, ); return c.json(result, result.ok ? 200 : 400); }); diff --git a/web/src/api/github.ts b/web/src/api/github.ts index fb45cb4e..1a63ea3d 100644 --- a/web/src/api/github.ts +++ b/web/src/api/github.ts @@ -97,10 +97,13 @@ export async function getGitHubPrReviewThreads( { repo: input.repo, prNumber: input.number }, options, ); + const reviewThreads = response.data?.reviewThreads ?? []; return { - reviewThreads: response.data?.reviewThreads ?? [], + reviewThreads, reviewThreadsTruncated: response.data?.reviewThreadsTruncated ?? false, - unresolvedReviewThreads: response.data?.unresolvedReviewThreads ?? [], + unresolvedReviewThreads: + response.data?.unresolvedReviewThreads ?? + reviewThreads.filter((thread) => !thread.isResolved), }; } From be52965b53eef66d42bb51153687067d78460160 Mon Sep 17 00:00:00 2001 From: syn Date: Sat, 18 Jul 2026 08:29:55 -0500 Subject: [PATCH 2/2] Invalidate review thread cache after mutation attempts --- src/modules/pr-events/service.ts | 40 ++++++++++++-------- src/pr-event-state.test.ts | 64 ++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 16 deletions(-) diff --git a/src/modules/pr-events/service.ts b/src/modules/pr-events/service.ts index 6f485114..3997858d 100644 --- a/src/modules/pr-events/service.ts +++ b/src/modules/pr-events/service.ts @@ -1044,16 +1044,20 @@ export async function postGitHubPrThreadReply( const replier = dependencies.replyToPullRequestReviewThread ?? replyToPullRequestReviewThread; - const thread = await replier({ - token, - threadId, - body: parsed.output.text, - }); - invalidatePullRequestReviewSurfaceThreadCache({ - owner: resolved.target.owner, - repo: resolved.target.repo, - number: resolved.target.number, - }); + let thread: GitHubPullRequestReviewThread; + try { + thread = await replier({ + token, + threadId, + body: parsed.output.text, + }); + } finally { + invalidatePullRequestReviewSurfaceThreadCache({ + owner: resolved.target.owner, + repo: resolved.target.repo, + number: resolved.target.number, + }); + } return okResult(action, true, 'Posted review thread reply.', { thread: thread as unknown as JsonValue, }); @@ -1127,12 +1131,16 @@ export async function postGitHubPrThreadResolution( resolvePullRequestReviewThread) : (dependencies.unresolvePullRequestReviewThread ?? unresolvePullRequestReviewThread); - const thread = await mutator({ token, threadId }); - invalidatePullRequestReviewSurfaceThreadCache({ - owner: target.target.owner, - repo: target.target.repo, - number: target.target.number, - }); + let thread: GitHubPullRequestReviewThread; + try { + thread = await mutator({ token, threadId }); + } finally { + invalidatePullRequestReviewSurfaceThreadCache({ + owner: target.target.owner, + repo: target.target.repo, + number: target.target.number, + }); + } return okResult( action, true, diff --git a/src/pr-event-state.test.ts b/src/pr-event-state.test.ts index 63283cb2..d74888ef 100644 --- a/src/pr-event-state.test.ts +++ b/src/pr-event-state.test.ts @@ -8,6 +8,10 @@ import type { GitHubPullRequestEventState, GitHubPullRequestReviewThread, } from './modules/github'; +import { + fetchPullRequestReviewSurfaceThreadsWithMetadata, + invalidatePullRequestReviewSurfaceThreadCache, +} from './modules/github'; import { getGitHubPrBranchPermissions, getGitHubPrFiles, @@ -910,6 +914,66 @@ describe('PR event state watermarks', () => { expect(resolveCalls).toEqual(['fetch:thread-1', 'resolve:thread-1']); }); + it('invalidates cached review threads when a mutation follow-up fails', async () => { + const home = await tempHome(); + const paths = runtimePaths(home); + await writeRepoRegistry(paths.repos); + const token = 'mutation-cache-token'; + const target = { + token, + owner: 'pandemicsyn', + repo: 'neondeck', + number: 123, + }; + const originalFetch = globalThis.fetch; + let fetchCalls = 0; + globalThis.fetch = async () => { + fetchCalls += 1; + return new Response( + JSON.stringify({ + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [], + }, + }, + }, + }, + }), + { status: 200, headers: { 'content-type': 'application/json' } }, + ); + }; + + try { + await fetchPullRequestReviewSurfaceThreadsWithMetadata(target); + await expect( + postGitHubPrThreadReply( + { repo: 'neondeck', prNumber: 123 }, + 'thread-1', + { text: 'Thanks, fixed.' }, + paths, + { + token, + fetchPullRequestReviewThread: async () => reviewThread(), + replyToPullRequestReviewThread: async () => { + throw new Error('Follow-up thread refresh failed.'); + }, + }, + ), + ).resolves.toMatchObject({ + ok: false, + action: 'github_pr_thread_reply_post', + }); + await fetchPullRequestReviewSurfaceThreadsWithMetadata(target); + expect(fetchCalls).toBe(2); + } finally { + globalThis.fetch = originalFetch; + invalidatePullRequestReviewSurfaceThreadCache(target); + } + }); + it('preserves omitted review draft fields on partial saves', async () => { const home = await tempHome(); const paths = runtimePaths(home);