feat(review): large-PR pipeline, instant-open checkout, scroll perf, and worker-pool highlighting#893
Merged
Merged
Conversation
Two PR-mode improvements: 1. Large GitHub PRs no longer fail to load. When `gh pr diff` is refused (HTTP 406 for oversized diffs), fetchGhPR pages through the pulls files API and stitches the per-file patches into a unified diff — mirroring the existing GitLab raw_diffs fallback. Path quoting matches git's exact rules (bare spaces unquoted) so downstream parsers round-trip; truncation at the API's 3000-file cap is surfaced, never silent. 2. The --local worktree/clone no longer blocks startup. The review server opens as soon as the platform diff arrives; the checkout warms in the background as a seeded not-ready pool entry. Consumers that need real files (agent jobs, full-stack diff, code-nav, semantic diff, AI sessions) await pool.ensure(), with creations serialized so concurrent fetches can't clobber the shared FETCH_HEAD. Cross-repo clone steps converted from spawnSync to async spawns; warmup children are killed on exit (plus `git worktree prune`) so aborted sessions can't leak stale registrations; failed checkouts degrade honestly (no agent runs in the wrong directory claiming local access) with a 30s retry cooldown.
…d renames Stress-testing against oven-sh/bun#30412 (2,188 files) surfaced three bugs: - Bun.serve's default 10s idleTimeout killed /api/semantic-diff while it parked on the background checkout warmup (a clone that can take minutes). Disable the idle timeout on all servers — AI SSE streams can also stall >10s between bytes while a permission prompt waits. - The file-badge hook memoized that failed fetch in a module-level cache keyed by patch, pinning every badge to empty until a hard refresh. Never cache failures; retry with backoff (5s/15s/30s). - reconstructGhPatch/reconstructPatch omitted the `similarity index` line, which Pierre's parser keys rename classification off — pure renames rendered as blank plain changes with no old path. Emit 100% for patch-less renames/copies (exactly accurate) and a synthetic 99% for patched ones (consumers only branch on 100% vs not).
…ated On oversized PRs the platform APIs withhold per-file patch content entirely (bun#30412: 1,066 of 2,188 files came back with status added/modified, zeroed counts, and no patch). Those files rendered as empty stubs with no diff. - fetchGhPR/fetchGlMR flag the result `patchIncomplete` when patch-less non-rename entries exist or the 3000-file cap truncates the listing. - New runPRLayerLocalDiff (pr-stack.ts) recomputes the exact layer diff in the local checkout: platform merge-base + head SHA two-dot diff (three-dot vs baseSha fallback), fetch-by-SHA for objects missing from shallow clones, -l0 so rename detection doesn't silently degrade on huge PRs. - The review UI shows a "Partial diff · Load full diff" notice in layer scope; clicking re-requests the layer scope and the server swaps in the recomputed full diff (waiting out the background clone if needed). - PR scope/switch state writes are epoch-guarded: a request parked on the checkout warmup can no longer overwrite a newer scope select or pr-switch. - draftKey follows the upgraded patch so annotation drafts survive pr-switch round-trips; recompute failures surface in the response error field. - Pi server mirrors all of it, including an agentCwd fallback so the upgrade works for PRs switched-to under a cross-repo clone pool.
backnotprop
changed the title
… detection External review caught a false negative: a too-large ADDED file comes back new_file:true with an empty diff — indistinguishable from a legitimately empty new file under the old heuristic, so the partial-diff upgrade was never offered for exactly the files that matter most on big MRs. The REST /diffs endpoint marks withheld content explicitly per entry (verified against gitlab.com): too_large/collapsed are now authoritative in both directions — withheld adds/deletes are flagged, binaries and empty files are never misflagged. Older GitLab without the fields keeps the empty-diff-on-modification heuristic.
…ff raw feedback The per-runtime defaults map (#627) gave OpenCode and Pi a different review-denied suffix than every other runtime; updating one meant the others silently kept "you must address all of them" — an instruction to start coding immediately. Claude Code, Amp, Droid, Codex, Copilot, Gemini, and Kiro were all still on it. One default for every runtime now: triage the feedback, verify it against the code, discuss before changing anything. Per-runtime customization remains available via config (prompts.review.runtimes.<rt>.denied), which resolves above the built-in default as before.
…xternal AI reviewers' Review feedback isn't always from AI reviewers or agent jobs; often it's the human reviewer's own annotations. Neutral wording covers both.
Self-review findings:
- The partial-diff upgrade reused the scope-switch handler, so clicking
"Load full diff" raised the full-screen PRSwitchOverlay — blocking the
entire UI, potentially for minutes behind a cold clone, with no text and
no cancel. The upgrade now has its own loading state: the notice shows a
spinner ("Loading full diff…") and the reviewer keeps working with the
partial diff while the request parks. Server-side epoch guards already
handle scope/PR changes made during the wait.
- GitLab too_large/collapsed: treat explicit null like absent (flags
inconclusive → legacy heuristic decides) instead of silently exonerating.
- Rename-limit lift uses -l100000 instead of -l0 ("0 = unlimited" only
holds on git >= 2.29; on older git it could disable detection outright).
…iling The badge retry change (a2d19a4) cleared the client-side sem cache on failure so transient errors could recover. But file-header badges mount and unmount on every scroll in the virtualized all-files view, and each mount re-requests /api/semantic-diff — and the server only cached SUCCESSFUL runs. With sem erroring, scrolling spawned a continuous stream of sem processes, pegging the CPU and making scrolling severely choppy. Bound retry rate by time, not by mount events: - client: keep the failed result memoized and expire it after a 60s cooldown instead of clearing immediately - server (Bun + Pi): memoize failed sem runs for 30s in SemanticDiffResponseCache — request rate can no longer drive execution rate
…rom #885) The CodeView migration introduced severe scroll chop; scrolling UP could freeze the viewport entirely ("scrolling but nothing changes"). Three compounding causes, diagnosed against Pierre 1.2.8 source: 1. Lazy full-content augmentation landed updateItem() mid-scroll-gesture: the full-content parse counts collapsed-context regions the raw-patch parse doesn't, so the item GROWS — re-render + re-tokenize hitches both directions, and when the grown item sat above CodeView's scroll anchor, its corrective scrollTo() killed wheel momentum (the up-scroll freeze). Fetches still start as items enter the window; the item mutation now waits for 150ms of scroll quiet (staleness re-checked at apply time). 2. reportVisibleFile read container.scrollTop/clientHeight/scrollHeight on EVERY scroll event — a forced synchronous layout right after each frame's DOM writes. Replaced with CodeView's cached accessors and coalesced the handler to once per animation frame. 3. Missing containment CSS: Pierre's own production wrapper uses contain:strict + will-change:scroll-position so forced layouts stay scoped to the scroller instead of the whole document. Adopted. Also: __devOnlyValidateItemHeights now requires explicit opt-in (VITE_PIERRE_VALIDATE_HEIGHTS=1) — it runs getBoundingClientRect() per rendered item per frame and made dev-server scrolling choppy by itself.
Adopts two diffshub practices identified in the architecture comparison: - DiffFile now carries a derived status (added/deleted/renamed/modified) from the chunk's git metadata lines. FileHeader shows a status icon and renders renames as "old/path → new/path" (dimmed old, arrow — diffshub's treatment, including its rename blue); the file tree shows A/D/R markers. 'modified' is deliberately undecorated so the others pop. Works in both the all-files surface and the single-file panel, including header-only pure renames from the large-PR reconstruction. - CodeView container gains diffshub's remaining perf CSS: overflow-anchor: none (native scroll anchoring fights CodeView's own anchor resolution whenever item heights change — exactly our augmentation applies), overflow-x-clip, and overflow-clip containment on item elements.
A performance trace of scrolling a small local diff attributed 2.2s of
2.6s main-thread CPU to findNextMatchSync — shiki's TextMate regex
scanner tokenizing on the main thread. diffshub avoids this entirely by
running tokenization in Pierre's worker pool; we never opted in.
Wires WorkerPoolContextProvider around the review app (pool size
min(cores-1, 3), 100-entry AST LRU, common languages preloaded), gates
the all-files surface on pool readiness with a 5s escape hatch (a dead
pool degrades to plaintext-then-highlight, never a blank view), and
syncs the UI theme pair into the long-lived pool.
Single-file build constraint solved with Vite's ?worker&inline (base64
blob worker) + worker.format 'es' with inlineDynamicImports — the
worker's lazy import("shiki/wasm") branch collapses into the bundle and
is never taken (shiki-js engine: the win is moving work off the main
thread, with no .wasm asset to smuggle into one HTML file). Bundle
+850KB.
…ions A failed round-trip recorded the theme as synced and never retried, pinning the pool to the wrong palette for the session.
backnotprop
changed the title
…ssing checkout Dogfood review of this PR (via plannotator itself) caught two valid issues: - prPatchIncomplete was gated on the worktree pool, so a --no-local session showed a truncated diff with no indication at all. Partiality is information; upgradability is a capability. The flag is now always reported, with a separate prPatchUpgradeAvailable — the UI shows the amber notice either way, with the "Load full diff" button only when a checkout can exist (otherwise a "re-run with --local" hint). - After a FAILED checkout warmup, Ask AI sessions and agent jobs fell back to process.cwd() (or a wrong revision on Pi) — running in the wrong tree instead of failing. Both launch points now refuse with a clear "Local PR checkout unavailable — retry shortly" error (503); the job handlers surface buildCommand refusals instead of mislabeling them "Invalid JSON". Bun and Pi mirrored. A third finding (sem availability stuck after warmup) was triaged invalid: the availability probe detects the sem binary, which is cwd-independent.
--local is a CLI remedy; OpenCode sessions have no such flag. Visible text states the fact, the tooltip carries the CLI guidance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Started as two PR-review-mode pain points (large GitHub PRs erroring at load; checkout setup blocking the browser), then grew through stress-testing into a broader review-mode hardening pass: platform-withheld diff content, severe all-files scroll jank (pre-existing on main from #885), worker-pool syntax highlighting, and change-type status display.
1. Large GitHub PRs load via a files-API fallback
gh pr diffis still the primary path. When GitHub refuses to render it (HTTP 406),fetchGhPRpages throughpulls/{n}/files --paginateand stitches the per-file patches back into a unified diff, mirroring the existing GitLabraw_diffs→/diffsfallback.gh.similarity indexline (Pierre's parser keys rename classification off it — without it pure renames rendered as blank changes). GitLab reconstruction got the same fix.parsePaginatedArraymoved topackages/shared/cli-pagination.ts.2. PR reviews open immediately — checkout warms in the background
The
--localworktree/clone is no longer awaited beforestartReviewServer(). The pool is seeded with aready:falseentry plus the in-flight warmup promise; consumers that need real files (agent jobs, full-stack scope, code-nav, semantic diff, AI sessions)await pool.ensure().FETCH_HEAD); warmup children tracked and killed at exit +git worktree prune;pool.cleanup()waits out in-flight creations; honest degradation on warmup failure (no confidently-wrong cwd).Bun.serveno longer kills parked requests at its 10s defaultidleTimeout; AI SSE streams that stall between bytes survive too.3. Partial-diff upgrade: local recompute when the platform withholds content
On oversized PRs the platform APIs withhold per-file patch content outright (bun#30412: 1,066 of 2,188 files with zeroed counts and no patch). The background clone has the data; the API doesn't.
fetchGhPR/fetchGlMRflagpatchIncomplete(GitLab uses its explicittoo_large/collapsedper-entry flags when present — verified against gitlab.com; older GitLab falls back to a heuristic).-l100000so rename detection never silently degrades — and swaps it in, preserving the reviewer's file. Non-blocking: the request can park minutes behind a cold clone while the reviewer keeps working with the partial diff.agentCwdfallback for cross-repo pr-switch.4. All-files scroll jank — eliminated (pre-existing on main, from #885)
Profiled and fixed three compounding causes, validated against Pierre 1.2.8 source:
scrollTokilled wheel momentum ("scrolling but nothing changes"). Applies now wait for 150ms of scroll quiet; fetches still start on visibility.reportVisibleFileread raw DOM layout each event; now uses CodeView's cached accessors, coalesced to once per frame.contain:strict,overflow-anchor:none,will-change:scroll-position, item-leveloverflow-clip).__devOnlyValidateItemHeights(per-itemgetBoundingClientRectper frame) now requires explicit opt-in.5. Worker-pool syntax highlighting (diffshub parity)
A Performance trace attributed 2.2s of 2.6s main-thread CPU during a few seconds of scrolling to
findNextMatchSync— shiki tokenizing on the main thread. Tokenization now runs in Pierre's worker pool (min(cores−1, 3) workers, AST LRU, preloaded languages, theme pair synced, readiness-gated mount with a 5s plaintext-degrading escape hatch). Single-file build constraint solved with Vite?worker&inline(base64 blob worker, ES format, dynamic imports collapsed); theshiki-jsengine avoids bundling a .wasm asset. Bundle +850KB.6. Change-type status in headers + file tree
DiffFilecarries a derived status from the diff's metadata lines. Headers show added/deleted/renamed icons and render renames asold/path → new/path(diffshub's treatment, including its rename blue); the tree shows A/D/R markers. 'modified' is deliberately undecorated so the others pop. Covers header-only pure renames from the large-PR reconstruction.7. Review-denied prompt unification
The per-runtime defaults map gave OpenCode/Pi a different review-denied suffix than every other runtime ("you must address all of them" — an instruction to start coding immediately). One default for all runtimes now: "This feedback came from review. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings." Config overrides (
prompts.review.runtimes.<rt>.denied) still resolve above it.Verification
patchIncompletedetection,runPRLayerLocalDiff, seeded-warmup pool, sem failure-memoization, diff-status derivation).bun run typecheckgreen incl. Pi (route parity maintained); review/hook/opencode builds pass.Known cosmetic trade-off: the
agentCwdfield in/api/diffmay reference the checkout path a few seconds before it exists on disk (server-side consumers all verify existence).