perf(chat): avoid mounting collapsed file diffs - #8316
Conversation
1a66d7a to
25e5ebc
Compare
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Reverses the deliberate "no hand-rolled header lookalike" decision to win real perf; the dual-header parity that decision protected is now guarded only by a manual fixture. Watch
Suggestions
[DESIGN-REVIEWED] 3b3d772 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of I have everything I need: the intent file, the authoritative patch, and the base-tree files ( First-Principles-Verdict: CONCERNS The perf fix is cause-level and measured, but two polish items — a 420px responsive rule and a fallback-size cap — ride along undeclared. What this change shipsIntent: stop closed file-change rows from building hidden, expensive diff renderers (fixes #8273) — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 3b3d772 |
UX Review (Fable 5, fork) — ✅ PASSUX-level review of The diff has full context now: the new lightweight header pixel-matches Pierre's compact header (36px band, 12px/18px font, same color-mix tint, 10px inline padding, 13px icons), controls keep their existing jobs (chevron = disclosure, filename = file-open, whitespace = toggle), the focus handoff across the mount swap is exhaustively tested, and the pulsing staged placeholder is gone entirely. The one experiential trade: with Pierre no longer pre-mounted, every cold first open shows the UX-Verdict: PASS Lightweight headers match Pierre's band pixel-for-pixel, every control keeps its job, and focus handoff across the swap is airtight. Suggestions
[UX-REVIEWED] 3b3d772 |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsBoth candidates die under falsification: Candidate 1 (focus stranded when Pierre chunk fails): the claimed "wrong outcome" is not wrong. If the lazy chunk never resolves, Candidate 2 (extra scroll from Neither reaches the 80+ bar, and both are UX/a11y degradations outside the BLOCKING classes regardless. I found no additional grounded defect in the focus state machine — the edge cases (cold mount, lazy-never-resolves proxy retention, blur-away, null-target blur, reopen-during-collapse) are each covered by the added tests. No findings. [OPUS-REVIEWED] 3b3d772 |
25e5ebc to
3011d65
Compare
Removed both the fixed |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
350f1a4 to
1d95dc7
Compare
1d95dc7 to
0aee2d1
Compare
The row now cancels both pending handoffs whenever blur does not demonstrably remain within the row, including |
2dee1ff to
e3b8eb5
Compare
e3b8eb5 to
03dbc8d
Compare
03dbc8d to
331599a
Compare
331599a to
d6b0d40
Compare
d6b0d40 to
4e3f6ea
Compare
4e3f6ea to
b461af7
Compare
Render lightweight headers for closed file-change rows so transcript hydration does not initialize Pierre for hidden content. Preserve the existing collapse animation, disclosure behavior, focus handoff, and browser evidence.
b461af7 to
3b3d772
Compare
bolichen97
left a comment
There was a problem hiding this comment.
Reviewed the full diff. renderPierre = open || closing guarantees the diff mounts on every open path, focus handoff is bidirectional and abandon-safe, and mount-count tests fail if a collapsed row constructs Pierre. Two cosmetic drifts only: diffstat cells hidden below 420px on artifact rows, and a fluid metadata rail instead of the fixed 124px column. Approving.
Problem / Motivation
Collapsed file-change rows mounted one
PierreFilePairper file with the complete before/after contents. Pierre'scollapsedoption changed the output to a header, but it did not avoid loading or constructing the diff renderer.A mounted assistant message could therefore initialize eight hidden Pierre surfaces by default, with more added when the user selected "Show more."
Why it matters
A client performance trace showed Pierre/Shiki work dominating renderer long tasks, including multi-second main-thread stalls. Closed file rows should not parse, highlight, or construct content the user has not requested.
The waste also occurs in virtualizer overscan, nonvirtualized chat hosts, and the hidden local dashboard retained while viewing a remote instance.
What changed (motivation → approach → change)
The mount decision now lives in
FileChangeChips, which owns row disclosure.PierreFilePairmounts only while a row is open or completing its existing 180 ms collapse animation.Tool-derived diff cards are unchanged because they are visible results by design. Prose diff fences are unchanged because they already avoid mounting Pierre while closed.
PR #7916 overlaps
FileChangeChipswith staged mounting. This PR is intentionally standalone againstmain: staging spreads initialization over time but still eventually mounts closed Pierre rows, while this fix enforces the stricter lifecycle invariant that closed rows contain no Pierre subtree. Draft PR #4758 touches a separate section of the shared capture fixture; this PR updates only the file-row frames and selectors required by the new lifecycle.Tests
The broad backend suite is host-red on clean main in unrelated path-ownership, real-home isolation, AF_UNIX length, and worker-budget tests. On this branch it reached 84,055 passes, 330 skips, 101 failures, and 2 errors without any backend diff. Electron reached 1,561 passes and zero failures, then cancelled 26 descendants after the existing
gateway-stoppending-promise test. The affected website, static, and build gates are green.Manual verification
The updated real-Chromium capture fixture verified:
The fixture later timed out on an unrelated pre-existing prose-diff frame after these three assertions and screenshots completed.
Screenshots / video
Closed rows: zero Pierre mounts
320px viewport: filename remains useful
One row expanded
Lightweight filename hover after collapse
Related Issues
Related: #8241
Overlaps: #7916 and #4758, as described above.
Pattern harvest
Rule candidate: review-prompt
Pattern: A collapsed disclosure must conditionally omit an expensive child; passing a
collapsedprop to the child does not prevent initialization or hidden work.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)Fixes #8273