perf(chat): bound Pierre file-pair rendering - #8328
Conversation
9fd5ea9 to
5cbc0f6
Compare
5cbc0f6 to
be92ecb
Compare
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound budget at the right boundary, but the degradation is a hard cliff: files just over 400 lines lose the diff entirely, with no user override. Watch
Suggestions
[DESIGN-REVIEWED] 83a6499 |
UX Review (Fable 5, fork) — 🟡 CONCERNSUX-level review of UX-Verdict: CONCERNS Solid degraded-view design, but the live-edit budget flips per keystroke — a mid-typing editor swap with no notice — and no first-time reader has seen the fallback. Watch
Evidence gaps
Suggestions
[UX-REVIEWED] 83a6499 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All counts are verified. The base First-Principles-Verdict: CONCERNS One rider: What this change shipsIntent: stop measured multi-second dashboard freezes when huge old/new file pairs render — a FIX (#8241, captured 4.4s/2.5s/2.4s traces).
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 83a6499 |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsThe candidate's claimed harm (lost caret/undo on the No Step-2 findings meet the (a)/(b)/(c) bar: the budget predicate, fallback rendering, and locale additions are all internally consistent and grounded in the diff. No findings. [OPUS-REVIEWED] 83a6499 |
Bypass synchronous Pierre diff construction for oversized old/new source pairs while preserving complete content, editing, header controls, and responsive chat-row scrolling.
be92ecb to
83a6499
Compare
bolichen97
left a comment
There was a problem hiding this comment.
Tech Lead review: approved.
The bound is correct and self-limiting. isPierreFilePairWithinBudget tests the O(1) length ceiling (131,072 UTF-16 code units) before counting lines, so hasAtMostLines can never scan more than 128Ki charCodeAt calls -- the guard itself cannot become the cost it is preventing. Each side is bounded independently at 400 lines, and a null/null pair stays within budget, so the existing Pierre path is untouched for every input that was already cheap.
Placement is the only place it works. The check sits in the public PierreFilePair wrapper before the lazy chunk loads, which is what actually prevents the synchronous MultiFileDiff -> parseDiffFromFile -> createTwoFilesPatch -> processFile construction; a bound inside the impl would already have paid for it. All three call sites (FileChangeChips, DiffPanel, MarkdownPanel) inherit it from one wrapper rather than duplicating a limit.
Hook safety checked. The new early return precedes the rest of the component body, so I read index.tsx at head to confirm PierreFilePair contains no hooks at all (usePlainDiff lives in PierrePatch and inside FilePairImpl). No conditional-hook hazard when a pair crosses the boundary between renders.
Large views degrade without losing content. Both sides are preserved byte-for-byte (the test asserts textContent equality, not truncation). Header parity is exact: showHeader = options?.disableFileHeader === false mirrors PIERRE_CODE_DEFAULTS.disableFileHeader: true, so the fallback neither gains nor drops chrome relative to Pierre at any call site, and split / unified / wrap / collapsed all derive from the same options. The new fallbackContentStyle prop is the right fix for the 376px scroller, since Pierre's unsafeCSS styles its shadow root and cannot reach a light-DOM fallback.
Performance claim holds up. The 4.40s / 2.49s / 2.41s tasks and the 90.2% long-task share come from a captured trace behind #8241, and the mechanism is consistent with the traced call chain. The 400-line threshold is documented in config.ts against measured numbers (~20 ms at 400, ~120 ms at 1,000) with stated headroom under a 100 ms long-task budget at 4x CPU slowdown -- a defensible constant, not a guess.
Scope is tight. 26 files reduce to 5 source files and ~165 real lines; 12 are single-key locale additions the i18n gate mandates, 4 are tests, 1 is docs, and temp-screenshots/ is an established repo convention (958 existing files), so the Design review's objection to committing them does not apply here. frontend-conventions.md correctly records the new invariant, which a cross-cutting renderer limit needs.
Accepted tradeoffs, tracked not blocked. A 401-line file with a one-line change loses the diff with no "load anyway" escape hatch -- the honest cost of a hard bound, declared in both the PR body and the conventions doc, and better addressed as a follow-up affordance than by widening the limit. The live-edit predicate re-evaluating per keystroke is a narrow edge (you must type or paste across 400 lines mid-session); Opus falsified the state-loss claim from in-repo evidence (editorOptions is a stable [] memo and the factory caches by options identity, so the Editor instance is reused), and the mode-flip keying is pre-existing behavior this PR adds one condition to. A translated notice on the editor's degraded path is worth a follow-up.
GPT 5.6 and Opus 4.8 both clean, 75/75 checks green, PR Readiness success, no BLOCK verdicts. Merging.
Problem / Motivation
Large raw old/new file pairs can block the dashboard renderer before Pierre's worker pool or row virtualizer can help. A captured client trace showed three Pierre/Shiki main-thread tasks lasting 4.40 seconds, 2.49 seconds, and 2.41 seconds while entering Chat, switching sessions, and rendering paginated history.
Pierre 1.3.5 constructs file-pair metadata synchronously through
MultiFileDiff -> parseDiffFromFile -> createTwoFilesPatch -> processFile. Worker-backed highlighting does not bound that initial work.Why it matters
The measured tasks accounted for 90.2% of long-task time, produced a 4.55-second visible freeze, and delayed one interaction by 2,545 ms. Code-heavy chats become unresponsive during navigation even when the destination response has not arrived yet.
What changed (motivation → approach → change)
The public
PierreFilePairwrapper now owns a layout-independent input budget before its lazy Pierre chunk loads.Large file — simplified viewstatus explains why highlighting and line-level diff controls are absent.Relationship to #7916
PR #7916 overlaps
FileChangeChips,PlainCodeFallback, and the Pierre wrapper as part of a broader history-pagination, staged-rendering, and phone-scroll rewrite.This PR is intentionally based directly on
main, not on #7916. The two changes address different costs. #7916 changes when transcript and diff work becomes reachable; this PR places a measured hard bound on synchronous raw file-pair construction whenever a pair is rendered. Expanded rows, side panels, and live-diff editing still need that bound after staged mounting or history pagination lands.Shipping the guard directly keeps a separate regression and rollback boundary for the measured freeze. Waiting for or stacking on #7916 would couple a focused renderer safety limit to a much larger in-flight history rewrite and duplicate its review dependencies. Any textual conflict can be resolved when main moves without changing this PR's policy or tests.
Tests
pierre.renderBudget.test.tspins exact line and UTF-16 code-unit boundaries, uneven pairs, and new-file behavior.PierreFilePair.budget.test.tsxproves oversized pairs never mount Pierre while preserving both files, header defaults, collapsed controls, keyboard regions, separators, and caller sizing.PierreEditorImpl.test.tsxproves oversized live diffs remain editable.FileChangeChips.headerSlots.test.tsxpins the shared 376 px fallback scroller.83a6499bcf4573c8357517267c052ff06aeedc16; current-head CI and reviews are rerunning.Manual verification
A real Chromium fixture rendered a synthetic 401-line pair and verified:
Screenshots / video
Desktop split view
Narrow stacked view
Related Issues
Fixes #8241
Possible overlap: #7916, as described above.
Pattern harvest
Rule candidate: review-prompt
Pattern: Worker offload does not bound synchronous preprocessing that runs before the worker request; expensive renderers need an app-owned input budget at their public boundary.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)