diff --git a/web/src/App.tsx b/web/src/App.tsx index 1ba16f2..95e8ff9 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -137,6 +137,16 @@ function resetCommentsLine(n: number): string { return `${threads} deleted — drafts, submitted, and resolved alike`; } +// Whether two comment lists say the same thing. Every event on the stream is +// answered with a refetch, and most of those events — the handshake the stream +// opens with, a comment resolved in a thread this page already shows that way — +// bring back a list that reads exactly as the one on screen. Keeping the array +// we already have in that case is what stops an event nobody can see from +// re-rendering the diff underneath whatever is being typed. +function sameComments(a: Comment[], b: Comment[]): boolean { + return a.length === b.length && JSON.stringify(a) === JSON.stringify(b); +} + function scrollToFile(path: string) { document.getElementById(`file-${path}`)?.scrollIntoView({ behavior: 'smooth' }); } @@ -401,7 +411,10 @@ export default function App() { }, [ctx, loadDiff]); const refetchComments = useCallback(() => { - api.comments().then(setComments).catch(() => {}); + api + .comments() + .then((cs) => setComments((prev) => (sameComments(prev, cs) ? prev : cs))) + .catch(() => {}); }, []); // Live updates. Every event on this stream is about this review — the stream is @@ -497,11 +510,19 @@ export default function App() { setStale(null); }, [stale]); + // What submitDraft needs but must not be re-identified by. Every file in the + // diff is handed this callback, so a new one on each draft is a re-render of + // the whole change set for a state change that concerns one line — see the + // memo on FileView. The draft and the context are read when the comment is + // actually submitted, which is the only moment either matters. + const latest = useRef({ draft, ctx }); + latest.current = { draft, ctx }; + // submitDraft creates the comment for the currently-open draft (line/range, // file, or review level). const submitDraft = useCallback( async (body: string) => { - const d = draft; + const { draft: d, ctx } = latest.current; if (!d) return; if (d.level === 'line') { await api.createComment({ @@ -521,9 +542,12 @@ export default function App() { setDraft(null); refetchComments(); }, - [draft, ctx, refetchComments], + [refetchComments], ); + // Stable for the same reason submitDraft is: it goes to every file. + const cancelDraft = useCallback(() => setDraft(null), []); + const submitReview = useCallback(async () => { const { submitted } = await api.submit(); refetchComments(); @@ -924,7 +948,7 @@ export default function App() { reveal={reveal} onSetViewed={setFileViewed} onStartDraft={setDraft} - onCancelDraft={() => setDraft(null)} + onCancelDraft={cancelDraft} onSubmitDraft={submitDraft} onChanged={refetchComments} /> diff --git a/web/src/components/DiffView.tsx b/web/src/components/DiffView.tsx index 7dbb137..d357730 100644 --- a/web/src/components/DiffView.tsx +++ b/web/src/components/DiffView.tsx @@ -1,7 +1,9 @@ import { + memo, useCallback, useEffect, useMemo, + useRef, useState, type ReactElement, type ReactNode, @@ -74,6 +76,69 @@ interface Props { // render the diff unhighlighted, as GitHub does for generated blobs. const MAX_HIGHLIGHT_BYTES = 512 * 1024; +// One file's comments, already sorted into the three places the file puts them. +// `open` counts every unresolved thread on the file, outdated ones included — +// it is the number on the file header's comment button. +interface FileComments { + line: Comment[]; + file: Comment[]; + stale: Comment[]; + open: number; +} + +// Shared by every file that has no comments at all, which in a change set this +// size is most of them. A fresh empty object per file would defeat the memo on +// FileView for no reason. +const NO_COMMENTS: FileComments = { line: [], file: [], stale: [], open: 0 }; + +// Group the review's comments by the file they belong to — and hand a file the +// same object back when nothing in its group moved. +// +// The identity matters as much as the grouping. A refetch answers every event on +// the stream with a brand-new array of brand-new objects, so grouping alone +// would still give all sixty files a prop they had never seen and re-render the +// whole diff — tens of milliseconds of blocked input, arriving at whatever +// moment an agent happened to post a comment. Comparing each group by value and +// reusing the previous one narrows that to the file the event was actually +// about. +function useCommentsByFile( + comments: Comment[], + outdated: ReadonlySet, +): ReadonlyMap { + const previous = useRef(new Map()); + + return useMemo(() => { + const byFile = new Map(); + for (const c of comments) { + const existing = byFile.get(c.file); + if (existing) existing.push(c); + else byFile.set(c.file, [c]); + } + + const kept = new Map(); + const grouped = new Map(); + for (const [file, cs] of byFile) { + // Whether the diff can still place a comment is part of what the file is + // being handed, so the outdated set is part of what makes a group equal. + const key = JSON.stringify(cs.map((c) => [c, outdated.has(c.id)])); + const before = previous.current.get(file); + const value = + before?.key === key + ? before.value + : { + line: cs.filter((c) => c.level === 'line' && !outdated.has(c.id)), + file: cs.filter((c) => c.level === 'file'), + stale: cs.filter((c) => c.level === 'line' && outdated.has(c.id)), + open: cs.filter((c) => c.status !== 'resolved').length, + }; + kept.set(file, { key, value }); + grouped.set(file, value); + } + previous.current = kept; + return grouped; + }, [comments, outdated]); +} + export function DiffView({ files, comments, @@ -91,6 +156,8 @@ export function DiffView({ onChanged, draft, }: Props) { + const commentsByFile = useCommentsByFile(comments, outdated); + if (files.length === 0) { return (
@@ -103,26 +170,36 @@ export function DiffView({ return ( <> - {files.map((file) => ( - c.file === filePath(file))} - outdated={outdated} - viewType={viewType} - draft={draft} - viewed={viewed.has(filePath(file))} - changed={changed.has(filePath(file))} - previous={snapshots.get(filePath(file)) ?? null} - reveal={reveal?.file === filePath(file) ? reveal.seq : null} - onSetViewed={onSetViewed} - onStartDraft={onStartDraft} - onCancelDraft={onCancelDraft} - onSubmitDraft={onSubmitDraft} - onChanged={onChanged} - /> - ))} + {files.map((file) => { + const path = filePath(file); + return ( + + ); + })} ); } @@ -148,11 +225,16 @@ interface DragState { head: number; } -function FileView({ +// Memoized because there are sixty of these and a rendered diff is most of the +// page's DOM. Everything above re-renders for reasons that concern one file at +// most — a comment arriving on the stream, a draft opening, the window being +// resized — and without this each of those costs a re-render of the entire +// change set. Every prop is either a primitive or an identity held stable for +// exactly that purpose; see useCommentsByFile, and `submitDraft` in App. +const FileView = memo(function FileView({ file, base, comments, - outdated, viewType, draft, viewed, @@ -167,9 +249,9 @@ function FileView({ }: { file: FileData; base: string; - comments: Comment[]; - outdated: ReadonlySet; + comments: FileComments; viewType: ViewType; + // The draft, but only when it is on this file — null otherwise. draft: DraftTarget | null; viewed: boolean; changed: boolean; @@ -222,13 +304,9 @@ function FileView({ // Comments still anchored in this diff render against their line (or, for // file-level ones, at the top of the file). The rest are outdated: kept, but // gathered above the code with a note, since the line they named is gone. - const lineComments = comments.filter( - (c) => c.level === 'line' && !outdated.has(c.id), - ); - const fileComments = comments.filter((c) => c.level === 'file'); - const staleComments = comments.filter( - (c) => c.level === 'line' && outdated.has(c.id), - ); + // Sorted into those three by useCommentsByFile, which is also what keeps them + // the same arrays from one render to the next. + const { line: lineComments, file: fileComments, stale: staleComments } = comments; // Fetch the base-side source so collapsed context can be expanded on demand. // Added files have no base version, so expansion is disabled for them. @@ -467,7 +545,7 @@ function FileView({ return () => window.removeEventListener('mouseup', onUp); }, [drag, lineKeyToChangeKey, onStartDraft, path]); - const openCount = comments.filter((c) => c.status !== 'resolved').length; + const openCount = comments.open; // The header's +/− always describe the whole change, whichever scope is on // screen: they are how big this file's part of the review is, and having them // shrink when you toggle would make the file look like it had been reverted. @@ -687,7 +765,7 @@ function FileView({ )} ); -} +}); // Lines revealed per click on a directional expander, as on GitHub. const CHUNK = 20;