From 6747d08232357f3a52ab10aadf8f6a3651583315 Mon Sep 17 00:00:00 2001 From: Jacek Tomaszewski Date: Mon, 31 Aug 2026 14:15:41 +0200 Subject: [PATCH 1/2] fix(cockpit): a chapter too big to draw opens folded, so a huge PR still reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening a 368-file PR put 611,783 DOM nodes on the page — 47,806 diff rows, a gutter button on each, 900,000 pixels of scroll — and the browser then spent its time on style and layout rather than on the review: 27fps and 56% of the main thread blocked while scrolling, and about 8% burnt at idle re-committing that tree every poll. A chapter over 2,000 diff lines now opens folded, with the count and the reason in its header. The line is measured, not guessed: across 314 chapters of real reviews only two are past it, so a normal walkthrough opens exactly as it did. The fold is a default, not a refusal — one click opens it. Deciding it while rendering rather than seeding it from an effect is the whole point: folding a chapter React has already drawn pays the cost the fold exists to avoid. So the state records only where the user disagreed with the default. Two things this turned up on the way: - the detail view kept the previous review's artifact until the new fetch landed, drawing one PR's chapters under another's URL — and, with the fold, drawing them unfolded - a chapter's open/closed state outlived the review it was set in, so folding "other changes" on one PR folded the unrelated "other changes" on the next Measured after: 611,783 nodes → 67,539, no transient peak, 119fps, no long tasks at all. Co-Authored-By: Claude Opus 5 (1M context) --- SPEC.md | 22 ++++++++++-- src/core/diff.test.ts | 15 ++++++++ src/core/diff.ts | 8 +++++ web/src/Detail.tsx | 81 +++++++++++++++++++++++++++++++++---------- 4 files changed, 106 insertions(+), 20 deletions(-) diff --git a/SPEC.md b/SPEC.md index 946d79a..222008a 100644 --- a/SPEC.md +++ b/SPEC.md @@ -499,7 +499,7 @@ review this rule cannot reach: the review that graded nothing at all and only asks questions. Severity is advisory: no code derives or enforces the verdict from it. The -cockpit only *points out* disagreement (§17.5), and the blocker count is shown +cockpit only *points out* disagreement (§17.6), and the blocker count is shown beside the verdict wherever there is room, because that is the fact a reader can check the verdict against. @@ -1386,7 +1386,25 @@ keeps ringing, since that machine's tap lands where nobody is looking. The favicon shows a dot exactly while the walkable set is non-empty, re-derived on every queue fetch so no screen can leave it stale. -### 17.5 Truth-Telling Surfaces +### 17.5 The Walkthrough + +Chapters render open — the walkthrough is the point of the page — with one +exception: a chapter holding more than 2,000 diff lines opens folded, and its +header MUST say so and say why ("34,961 diff lines, folded to keep the page +quick"). A rendered diff line is a table row and a dozen DOM nodes, so the +catch-all chapter of a 368-file PR is 600,000 of them: the browser then spends +its time on layout rather than on the review, and scrolling collapses. The +fold MUST be decided while rendering, not corrected afterwards — folding a +chapter that has already been drawn pays the whole cost it exists to avoid. + +The fold is a default, not a refusal: one click opens it, and the user's +choice stands for as long as they are on that review. It does not outlive the +review — a chapter opened or folded here MUST NOT carry its id (`__other` +above all) onto the next PR's page, and neither may the artifact itself: the +detail view clears it when the key changes, so no review is ever drawn under +another's URL. + +### 17.6 Truth-Telling Surfaces - The verdict cell shows recommendation + confidence; the blocker count is shown wherever there is room (detail chip, queue strip) as the checkable diff --git a/src/core/diff.test.ts b/src/core/diff.test.ts index d5359a6..df88a8d 100644 --- a/src/core/diff.test.ts +++ b/src/core/diff.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "vitest"; import { + diffLineCounts, newSideLineText, oldSideLineText, patchForFiles, @@ -86,3 +87,17 @@ describe("oldSideLineText", () => { expect(newSideLineText(SAMPLE).get("src/old.ts")!.size).toBe(0); }); }); + +describe("diffLineCounts", () => { + it("counts each file's diff lines, which is what the cockpit has to draw", () => { + const counts = diffLineCounts(SAMPLE); + expect([...counts.keys()]).toEqual(splitDiffByFile(SAMPLE).map((p) => p.path)); + for (const p of splitDiffByFile(SAMPLE)) { + expect(counts.get(p.path)).toBe(p.patch.split("\n").length); + } + }); + + it("has nothing to count in an empty diff", () => { + expect(diffLineCounts("").size).toBe(0); + }); +}); diff --git a/src/core/diff.ts b/src/core/diff.ts index 4c8843d..b6643ee 100644 --- a/src/core/diff.ts +++ b/src/core/diff.ts @@ -120,3 +120,11 @@ export function unclaimedFiles(diff: string, chapters: { files: string[] }[]): s .map((p) => p.path) .filter((p) => !claimed.has(p)); } + +/** + * How many diff lines each file contributes, by path. The cockpit renders one + * table row per line, so this is what a chapter costs a browser to draw. + */ +export function diffLineCounts(diff: string): Map { + return new Map(splitDiffByFile(diff).map((p) => [p.path, p.patch.split("\n").length])); +} diff --git a/web/src/Detail.tsx b/web/src/Detail.tsx index 9e9e200..6a2af46 100644 --- a/web/src/Detail.tsx +++ b/web/src/Detail.tsx @@ -1,7 +1,7 @@ import { html } from "diff2html"; import { ReactNode, useEffect, useMemo, useRef, useState } from "react"; import { createPortal } from "react-dom"; -import { patchForFiles, splitDiffByFile, unclaimedFiles } from "../../src/core/diff"; +import { diffLineCounts, patchForFiles, splitDiffByFile, unclaimedFiles } from "../../src/core/diff"; import { withGrade } from "../../src/core/severity"; import { addComment, @@ -585,12 +585,25 @@ function AddComment({ ); } +/** + * Beyond this many diff lines a chapter opens folded rather than rendered. + * + * The cockpit draws one table row per diff line and a dozen DOM nodes per row, + * so a catch-all chapter holding a 368-file PR lands 600,000 nodes on the page + * and the browser spends its time on style and layout instead of on the + * review. Measured over 314 chapters of real reviews, 2 are past this line — + * a normal walkthrough opens exactly as it did, and the ones that would grind + * are one click away. + */ +const foldChapterOverLines = 2000; + function ChapterSection({ chapter, n, diff, comments, open, + heavy, onToggle, onUpdateComment, onDeleteComment, @@ -607,6 +620,8 @@ function ChapterSection({ diff: string; comments: ReviewComment[]; open: boolean; + /** Diff lines, when there are too many of them to have opened with. */ + heavy: number | null; onToggle: () => void; flash: string | null; onUpdateComment: (id: string, patch: { body?: string; status?: string }) => void; @@ -649,6 +664,11 @@ function ChapterSection({ {comments.length > 0 ? ` · ${comments.length} comment${comments.length === 1 ? "" : "s"}` : " · no comments"} + {heavy != null && ( + + {` · ${heavy.toLocaleString()} diff lines, folded to keep the page quick`} + + )} {onDiscuss && ( @@ -1317,8 +1337,13 @@ export function Detail({ reviewKey }: { reviewKey: string }) { // reach the one chat panel at the bottom. const [chatRefs, setChatRefs] = useState([]); const chatInput = useRef(null); - // Chapters are open by default — the walkthrough is the point of the page. - const [collapsed, setCollapsed] = useState>(new Set()); + // Chapters are open by default — the walkthrough is the point of the page — + // except for one too big to draw (see foldChapterOverLines). This records + // only the chapters the user has since flipped the other way, so the default + // is decided while rendering rather than corrected after it: seeding it from + // an effect would draw the giant diff once before folding it away, which is + // the whole cost the fold exists to avoid. + const [flipped, setFlipped] = useState>(new Set()); const [focused, setFocused] = useState(0); const chapterEls = useRef>(new Map()); const verdictEl = useRef(null); @@ -1354,6 +1379,10 @@ export function Detail({ reviewKey }: { reviewKey: string }) { // Walking to another review starts at its top, not wherever the last one // left the page. window.scrollTo({ top: 0 }); + // Nothing of the last review outlives its URL. Keeping it until the fetch + // lands renders the wrong PR's chapters under this one's key — briefly + // drawing a diff this page has no business drawing. + setArtifact(null); setFreshness(null); setFreshnessError(null); setEventOverride(null); @@ -1458,15 +1487,37 @@ export function Detail({ reviewKey }: { reviewKey: string }) { : artifact.chapters; }, [artifact]); + /** How many diff lines each chapter is asking the browser to draw. */ + const weights = useMemo(() => { + const counts = diffLineCounts(artifact?.diff ?? ""); + return new Map( + chapters.map((ch) => [ch.id, ch.files.reduce((n, f) => n + (counts.get(f) ?? 0), 0)]), + ); + }, [artifact?.diff, chapters]); + /** Lines, when there are enough of them that this chapter opens folded. */ + const heavy = (id: string) => { + const lines = weights.get(id) ?? 0; + return lines > foldChapterOverLines ? lines : null; + }; + const isOpen = (id: string) => (flipped.has(id) ? heavy(id) != null : heavy(id) == null); + const flip = (id: string) => + setFlipped((s) => { + const next = new Set(s); + if (next.has(id)) next.delete(id); + else next.add(id); + return next; + }); + + // A fold belongs to the review it was made in. The cockpit walks from one + // review to the next without remounting, so without this a chapter opened + // here would carry its id — "__other" above all — onto the next PR's page. + useEffect(() => setFlipped(new Set()), [reviewKey]); + const openChapter = (i: number) => { const ch = chapters[i]; if (!ch) return null; setFocused(i); - setCollapsed((s) => { - const next = new Set(s); - next.delete(ch.id); - return next; - }); + if (!isOpen(ch.id)) flip(ch.id); return ch; }; @@ -1787,7 +1838,7 @@ export function Detail({ reviewKey }: { reviewKey: string }) { {chapters.map((ch, i) => (