greyson/playpen
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Improve efficiency of review tool rendering.

Browse Source
This commit is contained in:
Greyson Parrelli
2026-08-28 10:07:48 -04:00
parent d7452c2262
commit 8ad7b3307d
2 changed files with 139 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
web/src/App.tsx
+28 -4
View File
@@ -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}
/>
web/src/components/DiffView.tsx
+111 -33
View File
@@ -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<string>,
): ReadonlyMap<string, FileComments> {
const previous = useRef(new Map<string, { key: string; value: FileComments }>());
return useMemo(() => {
const byFile = new Map<string, Comment[]>();
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<string, { key: string; value: FileComments }>();
const grouped = new Map<string, FileComments>();
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 (
<div className="empty-diff">
@@ -103,26 +170,36 @@ export function DiffView({
return (
<>
{files.map((file) => (
<FileView
key={filePath(file) + file.oldRevision + file.newRevision}
file={file}
base={ctx.base}
comments={comments.filter((c) => 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 (
<FileView
key={path + file.oldRevision + file.newRevision}
file={file}
base={ctx.base}
comments={commentsByFile.get(path) ?? NO_COMMENTS}
viewType={viewType}
// Only the file the draft is on hears about it. Every use of it
// inside FileView is already gated on the draft being this file's,
// so this changes nothing on screen — it just stops opening a
// composer from re-rendering the other sixty files.
draft={
draft && draft.level !== 'review' && draft.file === path
? draft
: null
}
viewed={viewed.has(path)}
changed={changed.has(path)}
previous={snapshots.get(path) ?? null}
reveal={reveal?.file === path ? reveal.seq : null}
onSetViewed={onSetViewed}
onStartDraft={onStartDraft}
onCancelDraft={onCancelDraft}
onSubmitDraft={onSubmitDraft}
onChanged={onChanged}
/>
);
})}
</>
);
}
@@ -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<string>;
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({
)}
</section>
);
}
});
// Lines revealed per click on a directional expander, as on GitHub.
const CHUNK = 20;