Skip to content

perf(chat): bound Pierre file-pair rendering - #8328

Merged
bolichen97 merged 1 commit into
kirodotdev:mainfrom
Pearcekieser:perf/pierre-file-pair-budget
Sep 6, 2026
Merged

perf(chat): bound Pierre file-pair rendering#8328
bolichen97 merged 1 commit into
kirodotdev:mainfrom
Pearcekieser:perf/pierre-file-pair-budget

Conversation

@Pearcekieser

@Pearcekieser Pearcekieser commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

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 PierreFilePair wrapper now owns a layout-independent input budget before its lazy Pierre chunk loads.

  • Pairs within 400 lines per side and 131,072 combined UTF-16 code units keep the existing Pierre renderer.
  • Larger pairs bypass Pierre and show both complete files in a themed plain view.
  • The fallback preserves header slots, collapsed rows, selection, wrapping, split and unified layouts, narrow stacking, and the existing 376 px chat-row scroller.
  • Scrollable source regions are keyboard reachable.
  • A translated Large file — simplified view status explains why highlighting and line-level diff controls are absent.
  • Oversized live-edit buffers remain editable through the normal file editor without live diff decoration.
  • Source-only rendering and already-computed unified patches are unchanged.

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.ts pins exact line and UTF-16 code-unit boundaries, uneven pairs, and new-file behavior.
  • PierreFilePair.budget.test.tsx proves oversized pairs never mount Pierre while preserving both files, header defaults, collapsed controls, keyboard regions, separators, and caller sizing.
  • PierreEditorImpl.test.tsx proves oversized live diffs remain editable.
  • FileChangeChips.headerSlots.test.tsx pins the shared 376 px fallback scroller.
  • Post-rebase focused run: 63 tests passed across 4 files.
  • TypeScript, ESLint, full i18n checks, and the production build passed.
  • Rebased conflict resolution validated locally on commit 83a6499bcf4573c8357517267c052ff06aeedc16; current-head CI and reviews are rerunning.
  • Two independent pre-publication reviews found six fallback parity and accessibility issues; all were fixed and reverified.

Manual verification

A real Chromium fixture rendered a synthetic 401-line pair and verified:

  1. The simplified view appears instead of Pierre rows.
  2. Both complete sides remain selectable.
  3. The content stays inside the 376 px internal scroller.
  4. Two keyboard-focusable source regions are present.
  5. Desktop uses side-by-side panes and narrow width stacks them.
  6. No page errors occur.

Screenshots / video

Desktop split view

Large file pair in the bounded desktop fallback

Narrow stacked view

Large file pair in the bounded narrow fallback

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

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@Pearcekieser
Pearcekieser requested a review from a team September 4, 2026 00:00
@Pearcekieser
Pearcekieser requested a review from a team as a code owner September 4, 2026 00:00
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@Pearcekieser
Pearcekieser force-pushed the perf/pierre-file-pair-budget branch 2 times, most recently from 9fd5ea9 to 5cbc0f6 Compare September 4, 2026 02:22
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@Pearcekieser
Pearcekieser force-pushed the perf/pierre-file-pair-budget branch from 5cbc0f6 to be92ecb Compare September 4, 2026 06:00
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 83a6499bcf4573c8357517267c052ff06aeedc16 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • No escape hatch on the cliff. A 401-line file with a one-line change costs ~20–120 ms to diff, yet the guard ("Larger pairs bypass Pierre and show both complete files") unconditionally replaces the diff with two full plain files — the user can no longer see what changed at all, for a file size agents produce routinely. GitHub-style precedent is a "Load diff anyway" action; here the loss is silent, permanent per pair, and not tunable by any setting, so the safety margin for a 4x-CPU worst case is paid by every normal-speed user on every moderately large file.
  • temp-screenshots/pierre-file-pair-budget/*.png land permanently in the repo. PR-description imagery committed to main is binary history that can never be pruned; attach the images to the PR instead and drop the directory from the diff.
  • In PierreEditorImpl, the budget predicate is live: a paste that crosses 400 lines mid-edit hard-swaps the diff surface for the plain editor with no equivalent of the fallback's translated status, so live-diff decoration vanishes without explanation.

Suggestions

  • Add a per-pair "Show full diff" affordance on the fallback that mounts Pierre on explicit click — a user-initiated freeze is acceptable, and it converts the cliff into a default rather than a ceiling.

[DESIGN-REVIEWED] 83a6499

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of 83a6499bcf4573c8357517267c052ff06aeedc16 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • Mid-edit budget crossing rebuilds the editor under the user's hands: renderLiveDiff = diffBase !== undefined && isPierreFilePairWithinBudget(baseFile, file) re-evaluates on every keystroke (liveFile.contents = content in CodeEditor), so typing past 400 lines / 128Ki code units hard-swaps <MultiFileDiff key="diff"> for <File key="file"> — diff decoration vanishes silently, the edit session is rebuilt (cursor/scroll at risk), and deleting back across the boundary flips it again, so a file sitting at the limit flaps. Rare to hit, high impact when hit, persistent at the boundary. Fix: latch the decision once per edit session (or add hysteresis), and show the editor the same "Large file — simplified view"-style notice the chat fallback gets — today the editor's degradation is the only undisclosed one.

Evidence gaps

  • The whole fallback surface — the "Large file — simplified view" status, the − name / + name side headings, desktop split and narrow stacked layouts — exists only in the two screenshots this PR adds (temp-screenshots/pierre-file-pair-budget/desktop.png, narrow.png), which are not materialized in this fork lane and got no blind read. Push the branch to this repository to run it.
  • The collapsed and header-visible fallback states (chat row with prefix/suffix/metadata slots) appear in no screenshot.
  • The live-edit decoration-loss transition (typing across the budget) needs a recording (.gif/.mp4), not a screenshot.

Suggestions

  • In PlainFilePairFallback, pair the symbol-only section headings ({marker} {file.name}) with translated words ("− Before" / "+ After"): two identical filenames distinguished only by −/+ is the one element a cold reader must decode unaided.

[UX-REVIEWED] 83a6499

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 83a6499bcf4573c8357517267c052ff06aeedc16 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All counts are verified. The base fallbackClassName prop already carries caller sizing bounds at the exact call site where the new prop is added, temp-screenshots/ is an established convention (958 existing files), and the wrapper covers all three PierreFilePair call sites (FileChangeChips, DiffPanel, MarkdownPanel). Final review:

First-Principles-Verdict: CONCERNS

One rider: fallbackContentStyle is a second spelling of the existing fallbackClassName bounds prop, passed side-by-side with it, both saying 376px.

What this change ships

Intent: stop measured multi-second dashboard freezes when huge old/new file pairs render — a FIX (#8241, captured 4.4s/2.5s/2.4s traces).

  1. Pairs over 400 lines/side or 128K chars show a plain two-pane view instead of the highlighted diff — justified (measured freeze; wrapper covers all 3 call sites)
  2. Translated "Large file — simplified view" status on that view, 12 locales — justified (i18n gate mandates it)
  3. Oversized live-edit buffers lose live diff decoration, stay editable — justified, declared
  4. New fallbackContentStyle prop on PierreFilePair — duplicate of existing fallbackClassName (1 consumer, same call site passes both)
  5. Two measured budget constants in pierre/config.ts — justified (benchmark-derived, single owning module)
  6. New frontend-conventions.md section — mandated by the same-commit spec-sync invariant
  7. Two screenshots under temp-screenshots/ — matches repo convention (958 existing files there)

Watch

  • PierrePatch (index.tsx:220) still parses arbitrarily large pre-computed patch strings synchronously — 1 sibling of the root cause, declared out of scope by "already-computed unified patches are unchanged"; acceptable deferral, but the freeze class isn't fully closed.

Subtractions

  • Drop fallbackContentStyle (index.tsx, FileChangeChips.tsx:59 hunk): route the existing fallbackClassName — documented at base as "caller-specific … fallback text and bounds" and already "max-h-[376px] overflow-auto" at this call site — onto PlainFilePairFallback's content div. One sizing prop instead of two that must stay in sync (the call site now passes 376 in two spellings).

[FIRST-PRINCIPLES-REVIEWED] 83a6499

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 83a6499bcf4573c8357517267c052ff06aeedc16 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 83a6499

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 83a6499bcf4573c8357517267c052ff06aeedc16 via the fork AI-review pipeline; updated in place on each push.

Review details

The candidate's claimed harm (lost caret/undo on the MultiFileDiffFile swap) depends entirely on @pierre/diffs EditProvider/createEditor internals I cannot open. The in-repo evidence points the other way: editorOptions is a stable memo ([]), and the code comment states the factory "caches by options object identity," so the same Editor instance is reused across the surface swap — which would preserve state, not reset it. I cannot re-derive an observable wrong outcome (c) without assuming library code I did not open, and the mode-swap-remount keying (key="diff"/key="file") is pre-existing, deliberate behavior the PR only adds one condition to. The candidate is self-rated low and dies under falsification.

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

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
Bypass synchronous Pierre diff construction for oversized old/new source pairs while preserving complete content, editing, header controls, and responsive chat-row scrolling.
@Pearcekieser
Pearcekieser force-pushed the perf/pierre-file-pair-budget branch from be92ecb to 83a6499 Compare September 6, 2026 00:51
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention readiness: passed Eligible automated validation passed for the current revision and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention labels Sep 6, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bolichen97
bolichen97 merged commit 9d66d81 into kirodotdev:main Sep 6, 2026
76 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Large file-pair diffs can freeze the chat renderer

2 participants