Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/quick-review-threads.md
Original file line number Diff line number Diff line change
@@ -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.
23 changes: 17 additions & 6 deletions .plans/FRONTEND_STATIC_REVIEW_20260715.html
Original file line number Diff line number Diff line change
Expand Up @@ -871,6 +871,9 @@ <h2 id="summary-title">Executive summary</h2>
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.
</p>
</div>

Expand Down Expand Up @@ -920,10 +923,10 @@ <h3>Correctness risks</h3>
<article class="panel-block">
<h3>Performance risks</h3>
<p>
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.
</p>
</article>
</div>
Expand Down Expand Up @@ -993,7 +996,9 @@ <h2 id="performance-title">Performance first: discussion report</h2>
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.
</div>
Expand Down Expand Up @@ -1022,6 +1027,8 @@ <h2 id="performance-title">Performance first: discussion report</h2>
<tr><td>Real PR · request stability</td><td>1 thread request · 0 aborted patches</td><td>381 kB thread · 743 kB total API</td><td>Last API response: 2,782 ms</td><td>Duplicate and settlement churn eliminated</td></tr>
<tr><td>Real PR · active-patch priority</td><td>612 ms tree · 798 ms first patch</td><td>620 ms LCP · 0.0012 CLS</td><td>1,724 ms threads · zero long tasks</td><td>Completed · first patch passes all samples</td></tr>
<tr><td>Real PR · prioritized requests</td><td>1 thread request · 4 patch requests</td><td>381 kB thread · 724 kB total API</td><td>0 aborted · last API response 2,021 ms</td><td>Active read settles before background fanout</td></tr>
<tr><td>Real PR · lean threads, uncached</td><td>639 ms tree · 924 ms first patch</td><td>58 kB thread · 401 kB total API</td><td>1,511 ms threads · 16.8 ms post-response render</td><td>Payload fixed; GitHub latency remains</td></tr>
<tr><td>Real PR · lean threads, warm</td><td>642 ms tree · 934 ms first patch</td><td>530 ms traced LCP · 0.00 CLS</td><td>459 ms threads · 6.8 ms backend cache hit</td><td>Completed · warm thread target passes</td></tr>
<tr><td>Real PR · cold local / GitHub</td><td>4,978 / 3,894 ms</td><td>295 / 765 kB metadata</td><td>0 / 1 truncated files</td><td>Both miss cold target; local is complete</td></tr>
<tr><td>Production initial preload</td><td>194.6 kB gzip</td><td>157.7 kB entry · 21.6 kB ccount</td><td>Focused-review wrapper 0.77 kB gzip</td><td>Track; no budget miss declared</td></tr>
</tbody>
Expand All @@ -1047,7 +1054,10 @@ <h2 id="performance-title">Performance first: discussion report</h2>
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.
</div>

<div class="finding-list">
Expand Down Expand Up @@ -1352,6 +1362,7 @@ <h2 id="actions-title">Recommended sequence</h2>
<li class="completed-action"><span class="completion">COMPLETED · MEASUREMENT + FIRST WAVE</span> <strong>Performance measurement gate:</strong> 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.</li>
<li class="completed-action"><span class="completion">COMPLETED · REQUEST-PATH REMEDIATION</span> <strong>P1 · Real PR request path:</strong> discuss the measured duplicate review-thread identity and repeated local metadata work, then implement only the approved fixes and repeat <code>npm run bench:pr-review</code>. The stable thread key and bounded revision-metadata cache are implemented and verified on the retained target.</li>
<li class="completed-action"><span class="completion">COMPLETED · ACTIVE PATCH PRIORITY</span> <strong>P1 · Active patch priority:</strong> 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.</li>
<li class="completed-action"><span class="completion">COMPLETED · WARM THREAD PATH</span> <strong>P1 · Review-thread latency:</strong> 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.</li>
<li class="completed-action"><span class="completion">COMPLETED · COPY/DISCLOSURE</span> <strong>P2 · Copy/disclosure:</strong> row-specific action names and full-value affordances for operational text. Suggested command: <code>$impeccable clarify</code>.</li>
<li class="completed-action"><span class="completion">COMPLETED · PR #140</span> <strong>P2 · Editor state ownership:</strong> keep new-comment, draft-edit, and thread-reply bodies scoped to their editor generation so older async completions cannot clear newer work.</li>
<li class="completed-action"><span class="completion">COMPLETED · FINAL PASS</span> <strong>P3 · Final pass:</strong> tabular metrics, timer cleanup, operation-scoped status, stable finding IDs, and config tightening. Suggested command: <code>$impeccable polish</code>.</li>
Expand Down
69 changes: 64 additions & 5 deletions .plans/PR_REVIEW_PERF_PLAN.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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)`
Expand All @@ -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
Expand Down
Loading