Skip to content

perf(chat): avoid mounting collapsed file diffs - #8316

Merged
bolichen97 merged 1 commit into
kirodotdev:mainfrom
Pearcekieser:fix/hidden-diff-mount-perf-20260903
Sep 5, 2026
Merged

perf(chat): avoid mounting collapsed file diffs#8316
bolichen97 merged 1 commit into
kirodotdev:mainfrom
Pearcekieser:fix/hidden-diff-mount-perf-20260903

Conversation

@Pearcekieser

@Pearcekieser Pearcekieser commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Collapsed file-change rows mounted one PierreFilePair per file with the complete before/after contents. Pierre's collapsed option 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.

  • Closed rows render a lightweight header with the existing filename, change icon, counts, diffstat, artifact badge, tooltip, and file-open action.
  • PierreFilePair mounts only while a row is open or completing its existing 180 ms collapse animation.
  • Header whitespace delegates through the existing row click router; native and Pierre filenames remain file-open actions, and the chevron remains the keyboard disclosure control.
  • Durable disclosure and the shared split/unified preference remain unchanged.
  • Focus transfers in both directions. During a cold lazy load, the persistent row temporarily holds focus until Pierre's chevron mounts; collapse returns focus to the lightweight chevron.
  • The browser capture fixture requires zero Pierre headers while all file rows are closed.

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 FileChangeChips with staged mounting. This PR is intentionally standalone against main: 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

  • Mount-count coverage proves initial closed rows mount zero Pierre surfaces.
  • "Show more" reveals additional headers without mounting Pierre.
  • Opening one file mounts only that file.
  • Collapse retains Pierre through the closing animation, then unmounts it.
  • Reopening during collapse cancels the pending swap.
  • Pure and rendered click-routing tests cover native and Pierre header whitespace, filename actions, and chevron propagation.
  • Focus tests cover lightweight → Pierre, Pierre → lightweight, and the cold Suspense interval before Pierre's header resolves.
  • Full final website suite: 1,807 test files passed.
  • Final focused and Plain-diffs integration suites: 183 tests passed.
  • Current-head Fast Gate CI plus repository GPT and Opus reviews: passed with zero findings.
  • TypeScript, ESLint, production build, i18n, docs, formatting, mypy, portability, duplication, and bundle-size gates passed.
  • Local GPT and Opus reviews found the opening-focus, cold-Suspense focus, and bare-div click-handler gaps. All three were fixed and a final Opus verifier confirmed every finding closed with no new blockers.

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-stop pending-promise test. The affected website, static, and build gates are green.

Manual verification

The updated real-Chromium capture fixture verified:

  1. Five closed file rows render five lightweight headers and zero Pierre shadow headers.
  2. At the 320px viewport floor, the target filename retains 96px, metadata uses 45px, and Pierre remains unmounted.
  3. Opening one row mounts and displays its split diff.
  4. Collapsing restores the lightweight row and preserves the filename's file-open hover treatment.

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

Five lightweight collapsed file rows

320px viewport: filename remains useful

Collapsed file rows at the 320px viewport floor

One row expanded

One file row expanded into a split Pierre diff

Lightweight filename hover after collapse

Collapsed filename open action

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 collapsed prop to the child does not prevent initialization or hidden work.

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

Fixes #8273

@Pearcekieser
Pearcekieser requested a review from a team September 3, 2026 23:26
@Pearcekieser
Pearcekieser requested a review from a team as a code owner September 3, 2026 23:26
@github-actions github-actions Bot added readiness: checking Automated validation is still running fork Pull request from a fork (external contributor) labels Sep 4, 2026
@github-actions github-actions Bot added 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 fix/hidden-diff-mount-perf-20260903 branch from 1a66d7a to 25e5ebc Compare September 4, 2026 01:39
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 3b3d7722d99a0efba36f3251a78c0723820d0821 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

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

  • The base comment in fileChangeChipsCss.ts chose Pierre's own header for closed rows precisely so the header could not "drift from the inline chat blocks"; this PR reintroduces the lookalike (CollapsedRowHeader + RowMetadata) and swaps it against Pierre's shadow header at every open/collapse boundary. The old height contract (STAGED_ROW_HEIGHT_PX pin test) is deleted while the dependency survives as an unpinned min-h-[36px] — if Pierre's header height or layout drifts, every collapse ends in a per-row jump/flicker that only the manual capture fixture can catch. Pin the two headers' heights (and ideally the swap frame) in an automated test.
  • The focus-proxy state machine (focusProxy, two pending refs, tabindex/role juggling, onVisible plumbed through WarmSwap and PierreFilePair) is the recurring tax of that same two-implementations shape: chevron identity is lost at each swap, so continuity is rebuilt by hand. It is well-tested here, but expect similar machinery for any future affordance that must survive the swap.

Suggestions

  • Add the height-parity assertion to the capture fixture's collapse frame (compare row height before/after the Pierre→lightweight swap) so drift reddens in CI rather than shipping as a scroll jump.

[DESIGN-REVIEWED] 3b3d772

@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 3b3d7722d99a0efba36f3251a78c0723820d0821 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.

I have everything I need: the intent file, the authoritative patch, and the base-tree files (FileChangeChips.tsx, pierre/index.tsx, fileChangeChipsCss.ts, staging consumers, and the temp-screenshots convention, which the PR template documents as the sanctioned evidence path — not a rider). Consumer counts: PierreFilePair has 3 real consumers (FileChangeChips.tsx, DiffPanel.tsx:53, MarkdownPanel.tsx:808); only FileChangeChips passes the three new props. Here is the review.

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 ships

Intent: stop closed file-change rows from building hidden, expensive diff renderers (fixes #8273) — a FIX.

  1. Closed rows render a plain header; no hidden Pierre/Shiki work — justified (traced multi-second stalls)
  2. Opening mounts the diff; collapse unmounts it after the 180ms animation — justified
  3. Closed rows leave the staged-mount queue; pulsing placeholder and STAGED_ROW_HEIGHT_PX deleted — justified subtraction
  4. Keyboard focus follows the chevron across the header swap, with a temporary row proxy during cold loads — justified (the swap itself creates the focus loss)
  5. Artifact rows hide diffstat cells below 420px — rides along, undeclared in "What changed"
  6. Cold-open plain preview capped at 8,000 chars / 376px scroll — rides along, symptom-level
  7. Three new optional PierreFilePair/WarmSwap props — one consumer each (FileChangeChips), but they are the plumbing for items 4/6
  8. Closed filename tooltip now shows the path, not "Open … in side panel" — relabel, undeclared
  9. Capture fixture gains a 320px frame, an artifact-API stub, and hard zero-Pierre gates — justified evidence
  10. RFC doc updated in the same commit — mandated (AGENTS.md spec rule)

Watch

  • Item 5 is a new visibility rule the description's change list never names; it appears only in fixture comments and manual verification. Its zero option is parity with today's crowding, so it is separable polish inside a perf fix.
  • Item 6 patches the unbounded-fallback symptom for one caller: the default fallbackNode path still hands DiffPanel.tsx:53 and MarkdownPanel.tsx:808 full contents, and pierre/index.tsx keeps 5 other uncapped PlainCodeFallback sites (grep PlainCodeFallback text=). A caller-side cap is defensible (plain-diff mode renders the fallback as the final surface, so a global cap would truncate real content), but this fix's scope should be stated as one caller, not the mechanism.
  • CollapsedRowHeader is the hand-rolled header lookalike the deleted comment in fileChangeChipsCss.ts existed to forbid; the two spellings (Pierre's open header vs the light closed header) must now be kept in visual parity by hand — an accepted cost of the measured win, but a standing drift risk.

Subtractions

  • Defer the artifact narrow-viewport rule: drop max-[420px]:hidden, the data-fcc-secondary-metadata hook, fixture frame 01b-chips-card-320, and the /api/artifacts/session-docs stub — the perf fix lands whole without them, and they deserve their own declared change.

[FIRST-PRINCIPLES-REVIEWED] 3b3d772

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — ✅ PASS

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

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 PlainCodeFallback — raw new-file text (capped at 8,000 chars) — rather than a diff, for up to the 2.5s WarmSwap deadline. That's the established fallback pattern on every other Pierre surface in the product, and this PR adds the cap and bounded scroll to soften it, so it lands as a suggestion rather than a concern.

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

  • fallbackText={fc.after.slice(...)} in FileChangeChips.tsx means a cold first open of a "diff" briefly shows the whole new file's raw text (up to 2.5s WarmSwap deadline) — feed a plain unified-diff string (the changed hunks) as fallbackText instead, so the interim content matches what the chevron promised.

[UX-REVIEWED] 3b3d772

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] 3b3d772

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

Both 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, renderHeaderPrefix never mounts, so pierreToggleRef.current stays null and completeOpenFocus early-returns — but that leaves focusProxy true and tabIndex=-1/role="button" in place, which is exactly what keeps onRowKeyDown handling Enter/Space to toggle the row. The row stays fully keyboard-operable; the retained proxy is graceful degradation, not a defect. (c) fails.

Candidate 2 (extra scroll from el.focus() without preventScroll): the outcome is stated as "may scroll" / "possible extra scroll jump," and the chevron being focused sits in the header band that scrollIntoView({ block: 'nearest' }) is already revealing — the same target, so no competing motion is established. This is a "might," which the instructions require me to drop.

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

@Pearcekieser
Pearcekieser force-pushed the fix/hidden-diff-mount-perf-20260903 branch from 25e5ebc to 3011d65 Compare September 4, 2026 03:34
@Pearcekieser

Copy link
Copy Markdown
Contributor Author
  • Fixed metadata rail crushes filenames at 320px (span=0483c38db065) — Fixed.

The lightweight metadata group reserved a fixed 124px basis and could not shrink, leaving too little filename space at the 320px viewport floor.

Removed both the fixed flexBasis and shrink-0 from the lightweight metadata group. Added a regression that requires an unset basis and shrinkable rail. Real Chromium at 320px now measures a 96px filename area and 45px metadata group with zero collapsed Pierre headers; the new narrow.png frame is SHA-pinned in the PR body. The focused suites pass all 54 tests, and pinned GPT and Opus reviews passed the replacement head with no findings.

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • PR #7916 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7916: REBASE. The closest thing to a duplicate in this set: same user-visible problem, same file, same render branch, two incompatible answers. It is not DUPLICATE because the code changes are materially different (defer-and-reserve vs never-mount) and 7916's queue also serves code blocks. One strategy should be chosen deliberately before either merges. Files: website/src/components/FileChangeChips.tsx.
  • This PR is OVERLAPPING with PR #4758. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8316: MERGE_DISCUSSION. File-overlap candidate on the capture fixture only; separate frames, separate feature, separate source files. Files: website/scripts/capture-pierre-chat-diffs.mjs. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
  • This PR is OVERLAPPING with PR #8300. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8316: MERGE_DISCUSSION. Same subsystem, disjoint mechanism and file set; the two are complementary rather than competing. Files: website/src/pierre/PierreImpl.tsx. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@Pearcekieser
Pearcekieser force-pushed the fix/hidden-diff-mount-perf-20260903 branch 2 times, most recently from 350f1a4 to 1d95dc7 Compare September 4, 2026 09:59
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision 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 fix/hidden-diff-mount-perf-20260903 branch from 1d95dc7 to 0aee2d1 Compare September 4, 2026 12:02
@Pearcekieser

Copy link
Copy Markdown
Contributor Author
  • A late-resolving Pierre chunk can reclaim focus after a row's first open (span=99203e0180d1) — Fixed.

Pending open/collapse handoffs remained armed after focus moved outside the row, allowing delayed lazy-mount or collapse completion to reclaim focus.

The row now cancels both pending handoffs whenever blur does not demonstrably remain within the row, including relatedTarget: null. Four regressions cover explicit outside focus and null-target blur during both cold Pierre mounting and the collapse window; the existing normal open and collapse handoff tests remain green. Final focused suites pass all 58 tests, with TypeScript, ESLint, production build, and pinned GPT/Opus review green on the replacement head.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: passed Eligible automated validation passed for the current revision labels Sep 4, 2026
@Pearcekieser
Pearcekieser force-pushed the fix/hidden-diff-mount-perf-20260903 branch from 2dee1ff to e3b8eb5 Compare September 4, 2026 18:54
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@Pearcekieser
Pearcekieser force-pushed the fix/hidden-diff-mount-perf-20260903 branch from e3b8eb5 to 03dbc8d Compare September 4, 2026 19:35
@github-actions github-actions Bot added 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 fix/hidden-diff-mount-perf-20260903 branch from 03dbc8d to 331599a Compare September 4, 2026 21:48
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 4, 2026
@Pearcekieser
Pearcekieser force-pushed the fix/hidden-diff-mount-perf-20260903 branch from 331599a to d6b0d40 Compare September 4, 2026 22:48
@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 4, 2026
@Pearcekieser
Pearcekieser force-pushed the fix/hidden-diff-mount-perf-20260903 branch from d6b0d40 to 4e3f6ea Compare September 4, 2026 23:08
@github-actions github-actions Bot added readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: passed Eligible automated validation passed for the current revision 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 fix/hidden-diff-mount-perf-20260903 branch from 4e3f6ea to b461af7 Compare September 5, 2026 00:35
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Sep 5, 2026
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.
@Pearcekieser
Pearcekieser force-pushed the fix/hidden-diff-mount-perf-20260903 branch from b461af7 to 3b3d772 Compare September 5, 2026 01:39
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
@bolichen97
bolichen97 enabled auto-merge (squash) September 5, 2026 06:48

@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.

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.

@bolichen97
bolichen97 merged commit 02684f8 into kirodotdev:main Sep 5, 2026
78 of 84 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 5, 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.

Collapsed file-change rows mount Pierre and process hidden diffs

2 participants