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

Improve review tool viewed diffs.

Browse Source
This commit is contained in:
Greyson Parrelli
2026-08-27 11:47:26 -04:00
parent 1223499369
commit 11ebe07e4f
7 changed files with 746 additions and 80 deletions
Download Patch File Download Diff File Expand all files Collapse all files
web/src/App.tsx
+2 -1
View File
@@ -221,7 +221,7 @@ export default function App() {
// back unmarked instead of quietly staying checked.
const fingerprints = useMemo(() => fingerprintFiles(parsedFiles), [parsedFiles]);
const { viewed, changed, setFileViewed, clearViewed } = useViewedFiles(
const { viewed, changed, snapshots, setFileViewed, clearViewed } = useViewedFiles(
path,
ctx,
fingerprints,
@@ -861,6 +861,7 @@ export default function App() {
draft={draft}
viewed={viewed}
changed={changed}
snapshots={snapshots}
reveal={reveal}
onSetViewed={setFileViewed}
onStartDraft={setDraft}
web/src/components/DiffView.tsx
+166 -33
View File
@@ -25,6 +25,12 @@ import {
import { api } from '../api';
import type { Comment, DiffContext, DraftTarget, Side } from '../types';
import { anchorLine, changeKeyIndex, filePath, lineFor } from '../lib/anchor';
import {
countChanges,
interdiffHunks,
newSourceOf,
splitLines,
} from '../lib/interdiff';
import { languageForFile, refractorAdapter } from '../lib/language';
import { CommentThread } from './CommentThread';
import { Composer } from './Composer';
@@ -43,14 +49,20 @@ interface Props {
ctx: DiffContext;
draft: DraftTarget | null;
viewed: ReadonlySet<string>;
// Files that lost their viewed mark this session because their diff changed.
// Flagged in the header so the mark coming off doesn't look like a glitch.
// Files that lost their viewed mark because their diff changed. Flagged in the
// header so the mark coming off doesn't look like a glitch.
changed: ReadonlySet<string>;
// For each file that has moved since it was marked viewed, the contents it was
// last read at — the "before" side of its since-viewed diff (see lib/viewed).
// A file absent from here has no such diff to offer.
snapshots: ReadonlyMap<string, string>;
// Path of a file to force open, with a sequence number so re-requesting the
// same file counts as a new request. Set when a jump targets a thread inside
// a collapsed file.
reveal: { file: string; seq: number } | null;
onSetViewed: (file: string, viewed: boolean) => void;
// `source` is the file's contents at the moment it was marked, kept so a later
// visit can be diffed against it. Absent when they can't be reconstructed.
onSetViewed: (file: string, viewed: boolean, source?: string) => void;
onStartDraft: (d: DraftTarget) => void;
onCancelDraft: () => void;
onSubmitDraft: (body: string) => Promise<void>;
@@ -70,6 +82,7 @@ export function DiffView({
ctx,
viewed,
changed,
snapshots,
reveal,
onSetViewed,
onStartDraft,
@@ -101,6 +114,7 @@ export function DiffView({
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}
@@ -143,6 +157,7 @@ function FileView({
draft,
viewed,
changed,
previous,
reveal,
onSetViewed,
onStartDraft,
@@ -158,6 +173,7 @@ function FileView({
draft: DraftTarget | null;
viewed: boolean;
changed: boolean;
previous: string | null;
reveal: number | null;
onSetViewed: Props['onSetViewed'];
onStartDraft: (d: DraftTarget) => void;
@@ -172,6 +188,22 @@ function FileView({
const [collapsed, setCollapsed] = useState(viewed);
const [drag, setDrag] = useState<DragState | null>(null);
// Which diff this file is showing: everything since the base ref, or only what
// has moved since you last marked it viewed. A file with a snapshot behind it
// opens on 'since' — the reason it is back in front of you is that it changed,
// and the change is the part you haven't read. Everything else has no 'since'
// to show and stays on 'full'.
const [scope, setScope] = useState<'full' | 'since'>(
previous != null ? 'since' : 'full',
);
// A file can go stale while you are looking at it — an agent editing the work
// tree is the normal case here — so the default is re-applied when a snapshot
// appears, not just on mount.
const hasPrevious = previous != null;
useEffect(() => {
setScope(hasPrevious ? 'since' : 'full');
}, [hasPrevious]);
// A jump from the comments rail can target a thread inside a collapsed file;
// opening the file here is what puts that thread in the DOM for the scroll to
// find. Keyed on the request's sequence number, so clicking the same comment
@@ -215,13 +247,41 @@ function FileView({
};
}, [base, file.oldPath, file.type]);
const [hunks, expandRange] = useSourceExpansion(file.hunks, oldSource);
const canExpand = oldSource != null;
// Number of lines in the base file, ignoring the trailing newline so we don't
// count a phantom empty line at the end.
// The file as it stands now, rebuilt from the base source and the patch. It is
// what a viewed mark snapshots, and the "after" side of the since-viewed diff.
const newSource = useMemo(() => newSourceOf(file, oldSource), [file, oldSource]);
// The since-viewed diff, computed whether or not it is the one on screen: the
// toggle labels itself with how much moved, so it needs the answer either way.
// Only files that have gone stale have a snapshot at all, so this runs for a
// handful of files at most.
const sinceHunks = useMemo(() => {
if (previous == null || newSource == null) return null;
return interdiffHunks(splitLines(previous), splitLines(newSource));
}, [previous, newSource]);
// 'since' is a request, not a guarantee: the base source it needs may still be
// in flight. Until it lands the full diff is what's on screen, and the toggle
// says so rather than showing a scope the code below isn't rendering.
const showingSince = scope === 'since' && sinceHunks != null;
// Everything downstream — expansion, highlighting, anchoring — works off the
// hunks actually being shown and the source their old side belongs to. For the
// since-viewed diff that source is the snapshot, which is a complete copy of
// the file, so context expansion and whole-file tokenizing both still work.
const baseHunks = showingSince ? sinceHunks : file.hunks;
const expandSource = showingSince ? previous : oldSource;
const [hunks, expandRange] = useSourceExpansion(baseHunks, expandSource);
const canExpand = expandSource != null;
// Number of lines in the old-side file, ignoring the trailing newline so we
// don't count a phantom empty line at the end.
const totalOldLines = useMemo(
() => (oldSource != null ? oldSource.replace(/\n$/, '').split('\n').length : null),
[oldSource],
() =>
expandSource != null
? expandSource.replace(/\n$/, '').split('\n').length
: null,
[expandSource],
);
// Highlighting is done over the *whole* file, never over the visible hunks
@@ -238,8 +298,12 @@ function FileView({
const tokens: HunkTokens | undefined = useMemo(() => {
const lang = languageForFile(path);
if (!lang) return undefined;
const whole = file.type === 'add' ? undefined : (oldSource ?? undefined);
if (file.type !== 'add' && whole === undefined) return undefined;
// The since-viewed diff always has its whole old side to hand — that's what a
// snapshot is — so the "added file" exemption below only applies to the full
// diff, where an added file genuinely has no base.
const wholeAdd = !showingSince && file.type === 'add';
const whole = wholeAdd ? undefined : (expandSource ?? undefined);
if (!wholeAdd && whole === undefined) return undefined;
if (whole !== undefined && whole.length > MAX_HIGHLIGHT_BYTES) return undefined;
try {
return tokenize(hunks, {
@@ -253,10 +317,16 @@ function FileView({
} catch {
return undefined;
}
}, [hunks, path, oldSource, file.type]);
}, [hunks, path, expandSource, file.type, showingSince]);
// Map "side:line" -> react-diff-view change key, so we can attach widgets.
const lineKeyToChangeKey = useMemo(() => changeKeyIndex(hunks), [hunks]);
// Map "side:line" -> react-diff-view change key, so we can attach widgets. In
// the since-viewed diff the old side is a snapshot this browser made up, so
// only new-side lines — which are the working file's own, numbered exactly as
// the full diff numbers them — are offered as anchors.
const lineKeyToChangeKey = useMemo(
() => changeKeyIndex(hunks, showingSince),
[hunks, showingSince],
);
// rangeKeys returns the change keys of lines [start, end] on a side, used to
// highlight a selection or an existing comment's range.
@@ -277,10 +347,12 @@ function FileView({
);
// Group line comments (and the active line-draft composer) by change key. A
// range comment anchors to its end line. Anything that isn't outdated has a
// line in this diff by construction, so a missing key here would mean the
// anchor index and the rendered hunks disagreed — drop it rather than render
// the thread against the wrong line; it still shows in the comments rail.
// range comment anchors to its end line. In the full diff anything that isn't
// outdated has a line by construction, so a missing key would mean the anchor
// index and the rendered hunks disagreed — drop it rather than render the
// thread against the wrong line; it still shows in the comments rail. The
// since-viewed diff carries only the lines that moved, so misses there are
// ordinary, and counted below so the file can say how many it isn't showing.
const widgets = useMemo(() => {
const contentByKey: Record<string, ReactNode[]> = {};
@@ -308,6 +380,16 @@ function FileView({
return built;
}, [lineComments, lineKeyToChangeKey, draft, path, onChanged, onSubmitDraft, onCancelDraft]);
// Threads the since-viewed diff has no line for. They aren't lost — the rail
// still lists them, and the full diff still shows them in place — but a file
// that quietly drops half its comments when you toggle needs to say so.
const unplacedComments = useMemo(() => {
if (!showingSince) return 0;
return lineComments.filter(
(c) => !lineKeyToChangeKey[`${c.side}:${anchorLine(c)}`],
).length;
}, [showingSince, lineComments, lineKeyToChangeKey]);
// Highlight the lines being dragged, or the pending line-draft's range.
const selectedKeys = useMemo(() => {
if (drag) return rangeKeys(drag.side, drag.anchor, drag.head);
@@ -332,6 +414,11 @@ function FileView({
);
// Click-and-drag range selection on the gutter (GitHub style).
//
// The since-viewed diff's old gutter is inert: those numbers are positions in a
// snapshot only this browser ever held, so a comment anchored to one would be
// filed against a revision nothing else can find. Its new gutter behaves
// exactly as the full diff's does, because it carries the same line numbers.
const gutterEvents = useMemo(
() => ({
onMouseDown: (
@@ -340,6 +427,7 @@ function FileView({
) => {
if (!change) return;
const s = side ?? 'new';
if (showingSince && s === 'old') return;
const line = lineFor(change, s);
if (line == null) return;
e.preventDefault();
@@ -353,7 +441,7 @@ function FileView({
});
},
}),
[],
[showingSince],
);
// Finish a drag anywhere on the page: open a composer for the selected range.
@@ -380,14 +468,25 @@ function FileView({
}, [drag, lineKeyToChangeKey, onStartDraft, path]);
const openCount = comments.filter((c) => c.status !== 'resolved').length;
const additions = countChanges(file, 'insert');
const deletions = countChanges(file, 'delete');
// 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.
// What moved since you last looked is spelled out on the toggle instead.
const additions = countChanges(file.hunks, 'insert');
const deletions = countChanges(file.hunks, 'delete');
const sinceAdditions = sinceHunks ? countChanges(sinceHunks, 'insert') : 0;
const sinceDeletions = sinceHunks ? countChanges(sinceHunks, 'delete') : 0;
// Marking a file viewed folds it away, and unmarking brings it back — the
// reason you'd touch the checkbox is that you're done with (or returning to)
// this file, so the fold is the point.
//
// The contents go with the mark, so that when this file next changes there is
// something to diff against. `newSource` is null only for a binary file or one
// whose base contents haven't arrived; the mark is still worth making then, it
// just won't be able to show you what moved.
const toggleViewed = () => {
onSetViewed(path, !viewed);
onSetViewed(path, !viewed, newSource ?? undefined);
setCollapsed(!viewed);
};
@@ -431,6 +530,33 @@ function FileView({
</span>
)}
<span className="file-head-right">
{hasPrevious && !file.isBinary && (
<span className="segmented scope-toggle" role="group" aria-label="Diff scope">
<button
className={showingSince ? 'is-active' : ''}
onClick={() => setScope('since')}
disabled={sinceHunks == null}
aria-pressed={showingSince}
title={
sinceHunks == null
? 'Loading the version you last viewed…'
: `Only what moved since you marked this file viewed — +${sinceAdditions} −${sinceDeletions}`
}
>
<Icon name="clock" size={14} />
<span className="btn-label">since viewed</span>
</button>
<button
className={showingSince ? '' : 'is-active'}
onClick={() => setScope('full')}
aria-pressed={!showingSince}
title={`This file's whole diff against ${base}`}
>
<Icon name="file-diff" size={14} />
<span className="btn-label">full diff</span>
</button>
</span>
)}
<span className="file-stat file-stat-add">+{additions}</span>
<span className="file-stat file-stat-del">−{deletions}</span>
<DiffStat additions={additions} deletions={deletions} />
@@ -476,12 +602,29 @@ function FileView({
{staleComments.length > 0 && (
<OutdatedNote comments={staleComments} onChanged={onChanged} />
)}
{showingSince && (
<div className="scope-note">
<Icon name="file-diff" size={14} />
<span>
{sinceHunks?.length
? 'Showing what changed since you last marked this file viewed. The left side is the file as you read it then.'
: "Nothing in this file's contents moved since you last viewed it — the mark came off for something else, a rename or a mode change."}
</span>
{unplacedComments > 0 && (
<span className="muted">
{unplacedComments} comment
{unplacedComments === 1 ? '' : 's'} elsewhere in this file — switch
to the full diff to see them in place.
</span>
)}
</div>
)}
{file.isBinary ? (
<div className="binary-note">Binary file not shown.</div>
) : (
<Diff
className={drag ? 'is-dragging' : undefined}
diffType={file.type}
diffType={showingSince ? 'modify' : file.type}
viewType={viewType}
hunks={hunks}
tokens={tokens}
@@ -682,13 +825,3 @@ function DiffStat({
</span>
);
}
function countChanges(file: FileData, type: 'insert' | 'delete'): number {
let n = 0;
for (const hunk of file.hunks) {
for (const change of hunk.changes) {
if (change.type === type) n++;
}
}
return n;
}
web/src/lib/anchor.ts
+9 -1
View File
@@ -106,16 +106,24 @@ export function isOutdated(c: Comment, anchors: DiffAnchors | null): boolean {
// changeKeyIndex maps "side:line" -> react-diff-view change key for one file's
// hunks, so threads and composers can be attached as line widgets. Built from
// the hunks actually being rendered (expansion included), unlike buildAnchors.
//
// `newOnly` is for the since-viewed diff, whose old side is a snapshot of the
// file held in this browser rather than the base ref. Those line numbers are
// real, but they number a revision no comment was ever written against, so they
// must not be offered as anchors — indexing them would hang a thread meant for
// base line 40 off whatever line 40 of the snapshot happens to be.
export function changeKeyIndex(
hunks: readonly { changes: ChangeData[] }[],
newOnly = false,
): Record<string, string> {
const map: Record<string, string> = {};
for (const hunk of hunks) {
for (const change of hunk.changes) {
const key = getChangeKey(change);
const nl = lineFor(change, 'new');
const ol = lineFor(change, 'old');
if (nl != null) map[`new:${nl}`] = key;
if (newOnly) continue;
const ol = lineFor(change, 'old');
if (ol != null) map[`old:${ol}`] = key;
}
}
web/src/lib/interdiff.ts
+213
View File
@@ -0,0 +1,213 @@
// "What changed since I last looked at this file."
//
// A viewed mark records a fingerprint of the file's diff, and the mark comes off
// when that fingerprint moves (see lib/viewed). That tells you the file changed,
// but not *where* — the diff you come back to is the whole file against the base
// ref, with the two lines the agent just touched somewhere inside it.
//
// So a mark also stores the file's new-side contents at the moment it was made.
// Diffing that snapshot against the file as it stands now gives a second, much
// smaller diff: the file as you last read it on the left, the file as it is now
// on the right. That's what this module builds.
//
// The important property is that the new side of that diff *is* the current
// working file, numbered exactly as the real diff numbers it — so a comment left
// on a new-side line here lands on the same line as one left in the full diff,
// and no separate anchoring rules are needed. The old side is the snapshot, a
// revision of the file that only this browser ever knew about; nothing outside
// the rendered hunks may use its line numbers.
import type { ChangeData, FileData, HunkData } from 'react-diff-view';
import { diffLines } from './linediff';
// splitLines cuts source into lines, ignoring a trailing newline so a file that
// ends properly doesn't gain a phantom empty last line.
export function splitLines(source: string): string[] {
if (source === '') return [];
return source.replace(/\n$/, '').split('\n');
}
// newSourceOf reconstructs the file's new-side contents — the working copy —
// from the base-ref source plus the diff's hunks. Null when it can't be had: a
// binary file, or the base source not fetched yet.
//
// The base source is already in hand for every non-added file (DiffView fetches
// it for context expansion and highlighting), so this costs one pass over it and
// no extra request.
//
// One caveat, deliberately accepted: under `git diff -w` the hunks omit
// whitespace-only changes, so what comes out here is the working file with those
// changes left out. Both sides of the comparison are built the same way, and
// viewed marks are keyed by the whitespace toggle anyway, so the two halves
// always agree with each other.
export function newSourceOf(file: FileData, oldSource: string | null): string | null {
if (file.isBinary) return null;
if (file.type === 'delete') return '';
if (file.type === 'add') {
// An added file's hunks carry every line it has; there is no base to apply
// them to.
const lines: string[] = [];
for (const hunk of file.hunks) {
for (const change of hunk.changes) {
if (change.type !== 'delete') lines.push(change.content);
}
}
return lines.join('\n');
}
if (oldSource == null) return null;
const old = splitLines(oldSource);
const out: string[] = [];
// Next base line, 1-based, still to be copied across.
let cursor = 1;
for (const hunk of file.hunks) {
for (; cursor < hunk.oldStart && cursor <= old.length; cursor++) {
out.push(old[cursor - 1]);
}
for (const change of hunk.changes) {
if (change.type !== 'delete') out.push(change.content);
}
// max() rather than assignment: hunks are ordered and non-overlapping, but a
// malformed patch shouldn't be able to rewind the cursor and duplicate lines.
cursor = Math.max(cursor, hunk.oldStart + hunk.oldLines);
}
for (; cursor <= old.length; cursor++) out.push(old[cursor - 1]);
return out.join('\n');
}
// Context lines kept around each changed run, matching git's default.
const CONTEXT = 3;
interface Row {
change: ChangeData;
oldLine: number | null;
newLine: number | null;
}
// interdiffHunks diffs two revisions of a file into hunks react-diff-view can
// render. `before` is the snapshot, `after` the file now; new-side line numbers
// are `after`'s, which are the working file's own.
//
// Returns an empty array when the two are identical — which does happen, since a
// mark can be dropped by something the contents don't show: a rename, a mode
// change, a file that left the change set and came back.
export function interdiffHunks(before: string[], after: string[]): HunkData[] {
const rows: Row[] = [];
let oldLine = 0;
let newLine = 0;
for (const edit of diffLines(before, after)) {
if (edit.kind === 'eq') {
oldLine++;
newLine++;
rows.push({
change: {
type: 'normal',
isNormal: true,
content: after[edit.b],
oldLineNumber: oldLine,
newLineNumber: newLine,
},
oldLine,
newLine,
});
} else if (edit.kind === 'del') {
oldLine++;
rows.push({
change: {
type: 'delete',
isDelete: true,
content: before[edit.a],
lineNumber: oldLine,
},
oldLine,
newLine: null,
});
} else {
newLine++;
rows.push({
change: {
type: 'insert',
isInsert: true,
content: after[edit.b],
lineNumber: newLine,
},
oldLine: null,
newLine,
});
}
}
// Runs of changed rows, each padded with context and merged with its neighbour
// when the padding meets or the gap between them is a single line — splitting a
// hunk to hide one unchanged line helps nobody.
const groups: Array<[number, number]> = [];
for (let i = 0; i < rows.length; i++) {
if (rows[i].change.type === 'normal') continue;
const start = Math.max(0, i - CONTEXT);
const end = Math.min(rows.length - 1, i + CONTEXT);
const last = groups[groups.length - 1];
if (last && start <= last[1] + 1) last[1] = Math.max(last[1], end);
else groups.push([start, end]);
}
if (groups.length === 0) return [];
return groups.map(([start, end]) => {
const changes: ChangeData[] = [];
let oldStart = 0;
let oldLines = 0;
let newStart = 0;
let newLines = 0;
for (let i = start; i <= end; i++) {
const row = rows[i];
changes.push(row.change);
if (row.oldLine != null) {
oldLines++;
if (!oldStart) oldStart = row.oldLine;
}
if (row.newLine != null) {
newLines++;
if (!newStart) newStart = row.newLine;
}
}
// A hunk of nothing but insertions has no line on the old side to start at,
// so it starts after the last old line before it — the position git names in
// the same case.
if (!oldStart) oldStart = countBefore(rows, start, 'oldLine') + 1;
if (!newStart) newStart = countBefore(rows, start, 'newLine') + 1;
return {
content: `@@ -${oldStart},${oldLines} +${newStart},${newLines} @@`,
oldStart,
oldLines,
newStart,
newLines,
changes,
};
});
}
function countBefore(rows: Row[], index: number, side: 'oldLine' | 'newLine'): number {
let n = 0;
for (let i = 0; i < index; i++) {
if (rows[i][side] != null) n++;
}
return n;
}
// countChanges tallies one kind of change across hunks — the +/− shown against
// the "since viewed" toggle, and the same tally the file header uses.
export function countChanges(
hunks: readonly HunkData[],
type: 'insert' | 'delete',
): number {
let n = 0;
for (const hunk of hunks) {
for (const change of hunk.changes) {
if (change.type === type) n++;
}
}
return n;
}
web/src/lib/linediff.ts
+162
View File
@@ -0,0 +1,162 @@
// A line-level diff between two arrays of strings.
//
// It exists because the review pane sometimes has to diff two things git never
// saw together: the file as you last marked it viewed, and the file as it is now
// (see lib/interdiff). Both live in the browser — one in localStorage, one
// derived from the patch on screen — so there is nothing to ask the server for.
//
// The algorithm is Myers' O(ND) greedy edit-script search, the same one git uses
// by default. Written out here rather than pulled in as a dependency because it
// is sixty lines, and because the only alternative on hand is diff-match-patch,
// which react-diff-view happens to depend on for word-level marks — borrowing a
// transitive dependency for a different job is how a build breaks on an upgrade
// nobody connected to this file.
export type EditKind = 'eq' | 'del' | 'ins';
// One step of the edit script. `a` indexes the "before" array and `b` the
// "after"; the side an edit doesn't touch is -1.
export interface Edit {
kind: EditKind;
a: number;
b: number;
}
// Myers' cost is O((N+M)·D), so it's the *edit distance* that hurts, not the
// file size: a thousand-line file with three changed lines costs almost nothing.
// The cap is for the pathological case — two files that share no lines at all —
// where the search would grind through the full N+M diagonals to say so.
const MAX_D = 3000;
// Past this the trace alone would run to tens of megabytes. Nothing a reviewer
// reads is this big; a generated blob might be.
const MAX_LINES = 200_000;
// diffLines returns an edit script turning `a` into `b`, one entry per line of
// output. Every line of both inputs appears exactly once, in order.
export function diffLines(a: readonly string[], b: readonly string[]): Edit[] {
// Common head and tail are stripped first. Real edits are local, so this
// usually leaves Myers a handful of lines to work on rather than the file.
let head = 0;
while (head < a.length && head < b.length && a[head] === b[head]) head++;
let tailA = a.length;
let tailB = b.length;
while (tailA > head && tailB > head && a[tailA - 1] === b[tailB - 1]) {
tailA--;
tailB--;
}
const out: Edit[] = [];
for (let i = 0; i < head; i++) out.push({ kind: 'eq', a: i, b: i });
const midA = a.slice(head, tailA);
const midB = b.slice(head, tailB);
for (const e of diffMiddle(midA, midB)) {
out.push({
kind: e.kind,
a: e.a < 0 ? -1 : e.a + head,
b: e.b < 0 ? -1 : e.b + head,
});
}
for (let i = 0; tailA + i < a.length; i++) {
out.push({ kind: 'eq', a: tailA + i, b: tailB + i });
}
return out;
}
// diffMiddle handles the part left over once the shared head and tail are gone,
// where by construction the first and last lines differ.
function diffMiddle(a: readonly string[], b: readonly string[]): Edit[] {
if (a.length === 0 || b.length === 0) return replaceAll(a, b);
if (a.length + b.length > MAX_LINES) return replaceAll(a, b);
return myers(a, b) ?? replaceAll(a, b);
}
// replaceAll is the answer when there's no useful alignment to find, or when
// finding it would cost more than the result is worth: everything old goes,
// everything new arrives. Always correct, just coarse.
function replaceAll(a: readonly string[], b: readonly string[]): Edit[] {
const out: Edit[] = [];
for (let i = 0; i < a.length; i++) out.push({ kind: 'del', a: i, b: -1 });
for (let j = 0; j < b.length; j++) out.push({ kind: 'ins', a: -1, b: j });
return out;
}
// myers runs the greedy search, returning null if the edit distance exceeds the
// cap before a path is found.
//
// `v[offset + k]` is the furthest x reached on diagonal k = x - y. A copy of it
// is kept for every round so the path can be walked back afterwards; that's the
// whole memory cost, and why D is capped.
function myers(a: readonly string[], b: readonly string[]): Edit[] | null {
const n = a.length;
const m = b.length;
const bound = Math.min(n + m, MAX_D);
const offset = bound;
const v = new Int32Array(2 * bound + 2);
const trace: Int32Array[] = [];
for (let d = 0; d <= bound; d++) {
trace.push(v.slice());
for (let k = -d; k <= d; k += 2) {
// Extend the better of the two neighbouring diagonals. The `k === -d`
// test comes first so the read at `k - 1` can never go off the front.
const x =
k === -d || (k !== d && v[offset + k - 1] < v[offset + k + 1])
? v[offset + k + 1]
: v[offset + k - 1] + 1;
let y = x - k;
let head = x;
// Follow the snake: identical lines are free.
while (head < n && y < m && a[head] === b[y]) {
head++;
y++;
}
v[offset + k] = head;
if (head >= n && y >= m) return backtrack(trace, n, m, offset);
}
}
return null;
}
// backtrack walks the recorded rounds in reverse, turning the path into edits.
// `trace[d]` is the state as round d *began*, so it holds the (d-1)-step
// endpoints — which is exactly what the step taken in round d came from.
function backtrack(
trace: Int32Array[],
n: number,
m: number,
offset: number,
): Edit[] {
const edits: Edit[] = [];
let x = n;
let y = m;
for (let d = trace.length - 1; d >= 0; d--) {
const v = trace[d];
const k = x - y;
const prevK =
k === -d || (k !== d && v[offset + k - 1] < v[offset + k + 1]) ? k + 1 : k - 1;
const prevX = d > 0 ? v[offset + prevK] : 0;
const prevY = d > 0 ? prevX - prevK : 0;
// The snake that ended this round: matched lines, walked back.
while (x > prevX && y > prevY) {
x--;
y--;
edits.push({ kind: 'eq', a: x, b: y });
}
if (d === 0) break;
if (x === prevX) {
y--;
edits.push({ kind: 'ins', a: -1, b: y });
} else {
x--;
edits.push({ kind: 'del', a: x, b: -1 });
}
}
edits.reverse();
return edits;
}
web/src/lib/viewed.ts
+140 -45
View File
@@ -26,10 +26,23 @@ import type { DiffContext } from '../types';
//
// A mark records *what* was viewed, not just that it was: alongside each path we
// store a fingerprint of that file's diff at the moment it was marked (see
// lib/fingerprint). When the diff is reloaded and a file's fingerprint no longer
// matches, the mark is dropped — the code you signed off on isn't the code
// that's there now, so the file goes back in the pile, and `changed` reports it
// so the file doesn't just silently reappear.
// lib/fingerprint), and a snapshot of the file's contents at that moment. When
// the diff is reloaded and a file's fingerprint no longer matches, the mark stops
// counting as viewed — the code you signed off on isn't the code that's there
// now, so the file goes back in the pile.
//
// The mark itself isn't thrown away at that point: it is flagged stale and kept,
// and that flag is the whole record of a file having moved under the reviewer.
// It says two things. First, that the file needs looking at again, which is what
// `changed` reports and the file header shows — a file that quietly reappeared
// unmarked would read as a glitch. Second, for a mark that has a snapshot, it
// says what the file looked like when you read it, which is what lets a file
// offer "since viewed" alongside its full diff (see lib/interdiff); without that
// snapshot all the tool can say is *that* the file moved, never where.
//
// Being persisted, rather than remembered for the session, is what keeps those
// two in step: reloading the page must not leave a file offering a since-viewed
// diff while no longer saying why it is back in front of you.
const PREFIX = 'review-viewed';
@@ -38,8 +51,28 @@ const PREFIX = 'review-viewed';
// auto-unmarked. The next toggle replaces one with a real fingerprint.
const LEGACY = '';
// Marks maps a file path to the fingerprint of its diff when it was marked.
type Marks = Record<string, string>;
// Snapshots are the one part of a mark with no bound on its size, and
// localStorage is a handful of megabytes shared with everything else the pane
// keeps. Past this a file simply doesn't get one: it still marks viewed, still
// comes unmarked when it changes, still says so in its header, and only loses
// the "since viewed" toggle.
const MAX_SNAPSHOT = 256 * 1024;
// A mark on one file.
interface Mark {
// The file's diff fingerprint when the mark was made.
fp: string;
// The file's new-side contents at that moment, when small enough to keep.
src?: string;
// The fingerprint has since moved, so this is no longer a viewed mark: it says
// the file changed under the reviewer, and carries the "before" side of the
// since-viewed diff when it has one. An explicit flag rather than an inference
// from the fingerprint, so a mark can never come back to life because the code
// wandered back to a shape you once approved.
stale?: boolean;
}
type Marks = Record<string, Mark>;
// A ref can't contain a colon (git check-ref-format), so a suffix can never be
// mistaken for part of the base.
@@ -53,8 +86,9 @@ function keyFor(
return `${PREFIX}:${repo}:${ctx.base}${commit}${ignoreWhitespace ? ':w' : ''}`;
}
// load reads a repo's marks, accepting the older array-of-paths format that
// predates fingerprints.
// load reads a repo's marks, accepting both older formats: an array of paths,
// which predates fingerprints, and a path-to-fingerprint map, which predates
// snapshots.
function load(key: string | null): Marks {
if (!key) return {};
try {
@@ -62,14 +96,23 @@ function load(key: string | null): Marks {
if (Array.isArray(raw)) {
const out: Marks = {};
for (const path of raw) {
if (typeof path === 'string') out[path] = LEGACY;
if (typeof path === 'string') out[path] = { fp: LEGACY };
}
return out;
}
if (!raw || typeof raw !== 'object') return {};
const out: Marks = {};
for (const [path, fp] of Object.entries(raw as Record<string, unknown>)) {
if (typeof fp === 'string') out[path] = fp;
for (const [path, value] of Object.entries(raw as Record<string, unknown>)) {
if (typeof value === 'string') {
out[path] = { fp: value };
continue;
}
if (!value || typeof value !== 'object') continue;
const mark = value as Partial<Mark>;
if (typeof mark.fp !== 'string') continue;
out[path] = { fp: mark.fp };
if (typeof mark.src === 'string') out[path].src = mark.src;
if (mark.stale === true) out[path].stale = true;
}
return out;
} catch {
@@ -79,8 +122,27 @@ function load(key: string | null): Marks {
function save(key: string | null, marks: Marks) {
if (!key) return;
if (Object.keys(marks).length === 0) localStorage.removeItem(key);
else localStorage.setItem(key, JSON.stringify(marks));
if (Object.keys(marks).length === 0) {
localStorage.removeItem(key);
return;
}
try {
localStorage.setItem(key, JSON.stringify(marks));
} catch {
// Out of room, almost certainly because of the snapshots. Those are the
// expendable part of a mark, so drop every one of them and keep the marks
// themselves, stale flags included — losing your place in a review, or the
// note that a file moved, is much worse than losing the since-viewed diffs.
const lean: Marks = {};
for (const [path, mark] of Object.entries(marks)) {
lean[path] = mark.stale ? { fp: mark.fp, stale: true } : { fp: mark.fp };
}
try {
localStorage.setItem(key, JSON.stringify(lean));
} catch {
// Nothing left to give up. The marks stay in memory for this session.
}
}
}
// clearRepo drops every mark a repository has — under any base ref or commit, not
@@ -98,13 +160,15 @@ function clearRepo(repo: string | null) {
// (or the diff hasn't loaded yet), which says nothing about whether the work you
// reviewed changed — and if it comes back different, the fingerprint will say so
// then.
function isCurrent(marked: string, current: string | undefined): boolean {
return marked === LEGACY || current === undefined || current === marked;
function isCurrent(mark: Mark, current: string | undefined): boolean {
if (mark.stale) return false;
return mark.fp === LEGACY || current === undefined || current === mark.fp;
}
// useViewedFiles returns the viewed set for a review, the files that lost their
// mark because they changed, and a setter for one file. `fingerprints` is the
// current digest of each file in the diff on screen (see lib/fingerprint).
// mark because they changed, the snapshots those files were last read at, and a
// setter for one file. `fingerprints` is the current digest of each file in the
// diff on screen (see lib/fingerprint).
//
// localStorage is read back on every write, so two browser tabs on the same
// review each see the other's marks instead of clobbering the whole set.
@@ -116,14 +180,9 @@ export function useViewedFiles(
) {
const key = keyFor(repo, ctx, ignoreWhitespace);
const [marks, setMarks] = useState<Marks>(() => load(key));
// Files whose mark was dropped because their diff moved. Deliberately memory
// only: it's a "look again at this one" nudge for the session you're in, not a
// state worth resurrecting on reload.
const [changed, setChanged] = useState<ReadonlySet<string>>(new Set());
useEffect(() => {
setMarks(load(key));
setChanged(new Set());
}, [key]);
// Marks are shared by every tab on this origin, so another tab changing or
@@ -139,22 +198,26 @@ export function useViewedFiles(
return () => window.removeEventListener('storage', onStorage);
}, [key]);
// Persist the unmarking of files that changed. `viewed` below already ignores
// stale marks, so this isn't what makes them disappear from the UI — it's what
// stops one coming back to life later, when the same file's diff happens to
// match a fingerprint you signed off on two base refs ago.
// Flag the marks whose files have moved. `viewed` below already ignores those,
// so this isn't what makes them disappear from the UI — it's what records the
// change: that a file is owed a second read, and that a mark can't come back to
// life later when the same file's diff happens to match a fingerprint you
// signed off on two base refs ago.
//
// Nothing is deleted here, snapshot or not. A flagged mark with no snapshot is
// a path, a hash and a boolean, and it is what the file header reads to say the
// file changed under you — the same thing it says with one.
useEffect(() => {
if (!key || fingerprints.size === 0) return;
const stored = load(key);
const stale = Object.keys(stored).filter(
(path) => !isCurrent(stored[path], fingerprints.get(path)),
const moved = Object.keys(stored).filter(
(path) => !stored[path].stale && !isCurrent(stored[path], fingerprints.get(path)),
);
if (stale.length === 0) return;
if (moved.length === 0) return;
const next = { ...stored };
for (const path of stale) delete next[path];
for (const path of moved) next[path] = { ...next[path], stale: true };
save(key, next);
setMarks(next);
setChanged((prev) => new Set([...prev, ...stale]));
}, [key, fingerprints]);
// A mark whose file no longer matches is not a mark. Deciding this here rather
@@ -163,26 +226,59 @@ export function useViewedFiles(
// frame would have already mounted folded.
const viewed = useMemo(() => {
const out = new Set<string>();
for (const [path, fp] of Object.entries(marks)) {
if (isCurrent(fp, fingerprints.get(path))) out.add(path);
for (const [path, mark] of Object.entries(marks)) {
if (isCurrent(mark, fingerprints.get(path))) out.add(path);
}
return out;
}, [marks, fingerprints]);
// Files that have moved since they were marked viewed. Derived from the stale
// flag rather than tracked alongside it, so that what the header says and what
// the since-viewed toggle offers can never disagree — including across a reload,
// where a remembered-in-memory set would come back empty while the marks it was
// describing were still on disk.
const changed = useMemo(() => {
const out = new Set<string>();
for (const [path, mark] of Object.entries(marks)) {
if (mark.stale) out.add(path);
}
return out;
}, [marks]);
// The file as you last read it, for every file that has moved since. A subset
// of `changed`: this is the "before" side of a since-viewed diff, and a file too
// big to snapshot still reports that it changed, just not where.
const snapshots = useMemo(() => {
const out = new Map<string, string>();
for (const [path, mark] of Object.entries(marks)) {
if (mark.stale && mark.src != null) out.set(path, mark.src);
}
return out;
}, [marks]);
// setFileViewed marks or unmarks one file. `source` is the file's new-side
// contents right now — what a later visit will be compared against. Omitting
// it (a binary file, or one whose base contents haven't arrived) costs only the
// since-viewed diff.
//
// Unmarking deletes the whole mark, snapshot included: reopening a file you'd
// signed off on is a decision to read it again from the top, and keeping a
// "since" diff against the version you just rejected would be noise.
const setFileViewed = useCallback(
(path: string, next: boolean) => {
(path: string, next: boolean, source?: string) => {
const updated = load(key);
if (next) updated[path] = fingerprints.get(path) ?? LEGACY;
else delete updated[path];
if (next) {
const mark: Mark = { fp: fingerprints.get(path) ?? LEGACY };
if (source != null && source.length <= MAX_SNAPSHOT) mark.src = source;
updated[path] = mark;
} else {
delete updated[path];
}
save(key, updated);
setMarks(updated);
// Whichever way it was toggled, the file has just had the user's attention.
setChanged((prev) => {
if (!prev.has(path)) return prev;
const rest = new Set(prev);
rest.delete(path);
return rest;
});
// Whichever way it was toggled, the file has just had the user's attention:
// the mark it now has is either fresh or gone, so it is no longer stale and
// drops out of `changed` on its own.
},
[key, fingerprints],
);
@@ -192,8 +288,7 @@ export function useViewedFiles(
const clearViewed = useCallback(() => {
clearRepo(repo);
setMarks({});
setChanged(new Set());
}, [repo]);
return { viewed, changed, setFileViewed, clearViewed };
return { viewed, changed, snapshots, setFileViewed, clearViewed };
}
web/src/styles.css
+54
View File
@@ -1528,6 +1528,60 @@ body.is-resizing {
color: var(--muted);
}
/* ---- Diff scope toggle -------------------------------------------------- */
/* Which diff a file is showing: everything since the base ref, or only what has
moved since you last marked it viewed. The shared .segmented shell puts it in
the same shape as the split/unified control in the top bar, so it loses its
words on a narrow pane along with everything else. It appears only on files
that have both scopes to offer. */
.scope-toggle button {
display: inline-flex;
align-items: center;
gap: 6px;
color: var(--muted);
font-size: 12px;
font-weight: 500;
line-height: 1;
white-space: nowrap;
cursor: pointer;
}
.scope-toggle button:disabled {
opacity: 0.5;
cursor: default;
}
.scope-toggle button:disabled:hover {
background: transparent;
}
/* The selected segment carries the accent the hunk bands use, so "you are not
looking at the whole file" is visible from the same glance that reads the
file name — louder than .segmented's neutral is-active, because here the
choice changes what code is on screen rather than how it's laid out. */
.scope-toggle button.is-active {
background: var(--accent-soft);
color: var(--accent);
box-shadow: inset 0 0 0 1px var(--accent-muted);
}
/* Said once above the code, because a diff that silently hides most of a file
is worse than no toggle at all. Accent rather than attention yellow: this is
a view you chose, not something that went wrong. */
.scope-note {
display: flex;
align-items: baseline;
flex-wrap: wrap;
gap: 8px;
padding: 10px 16px;
background: var(--accent-soft);
border-bottom: 1px solid var(--border);
color: var(--text);
font-size: 13px;
}
.scope-note .octicon {
color: var(--accent);
align-self: center;
flex: none;
}
/* ---- Outdated comments -------------------------------------------------- */
/* Attention yellow, the same color the gutter uses for a commented line: the
comment is intact, only its anchor is gone. Nothing here reads as an error. */