diff --git a/SPEC.md b/SPEC.md index 946d79a..349dafa 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,26 @@ 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. Nor may anything else the last review put +on screen: the detail view clears the artifact and the load error when the key +changes, so no review is ever drawn under another's URL and no failure to load +one is reported over the next. + +### 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..82b0292 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 && !open && ( + + {` · ${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,11 @@ 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); + setError(null); setFreshness(null); setFreshnessError(null); setEventOverride(null); @@ -1458,15 +1488,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 +1839,7 @@ export function Detail({ reviewKey }: { reviewKey: string }) { {chapters.map((ch, i) => (