From 11ebe07e4f6a70e561e60fbbf620a2ead4fa9a62 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Thu, 27 Aug 2026 11:47:26 -0400 Subject: [PATCH] Improve review tool viewed diffs. --- web/src/App.tsx | 3 +- web/src/components/DiffView.tsx | 199 ++++++++++++++++++++++++----- web/src/lib/anchor.ts | 10 +- web/src/lib/interdiff.ts | 213 ++++++++++++++++++++++++++++++++ web/src/lib/linediff.ts | 162 ++++++++++++++++++++++++ web/src/lib/viewed.ts | 185 ++++++++++++++++++++------- web/src/styles.css | 54 ++++++++ 7 files changed, 746 insertions(+), 80 deletions(-) create mode 100644 web/src/lib/interdiff.ts create mode 100644 web/src/lib/linediff.ts diff --git a/web/src/App.tsx b/web/src/App.tsx index e223a7d..9a712a1 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -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} diff --git a/web/src/components/DiffView.tsx b/web/src/components/DiffView.tsx index 74b3f78..7dbb137 100644 --- a/web/src/components/DiffView.tsx +++ b/web/src/components/DiffView.tsx @@ -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; - // 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; + // 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; // 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; @@ -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(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 = {}; @@ -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({ )} + {hasPrevious && !file.isBinary && ( + + + + + )} +{additions} −{deletions} @@ -476,12 +602,29 @@ function FileView({ {staleComments.length > 0 && ( )} + {showingSince && ( +
+ + + {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."} + + {unplacedComments > 0 && ( + + {unplacedComments} comment + {unplacedComments === 1 ? '' : 's'} elsewhere in this file — switch + to the full diff to see them in place. + + )} +
+ )} {file.isBinary ? (
Binary file not shown.
) : ( ); } - -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; -} diff --git a/web/src/lib/anchor.ts b/web/src/lib/anchor.ts index 0018c15..6333f28 100644 --- a/web/src/lib/anchor.ts +++ b/web/src/lib/anchor.ts @@ -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 { const map: Record = {}; 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; } } diff --git a/web/src/lib/interdiff.ts b/web/src/lib/interdiff.ts new file mode 100644 index 0000000..9aa9a3c --- /dev/null +++ b/web/src/lib/interdiff.ts @@ -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; +} diff --git a/web/src/lib/linediff.ts b/web/src/lib/linediff.ts new file mode 100644 index 0000000..5ddc071 --- /dev/null +++ b/web/src/lib/linediff.ts @@ -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; +} diff --git a/web/src/lib/viewed.ts b/web/src/lib/viewed.ts index a7b3c8d..bfe4e9a 100644 --- a/web/src/lib/viewed.ts +++ b/web/src/lib/viewed.ts @@ -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; +// 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; // 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)) { - if (typeof fp === 'string') out[path] = fp; + for (const [path, value] of Object.entries(raw as Record)) { + if (typeof value === 'string') { + out[path] = { fp: value }; + continue; + } + if (!value || typeof value !== 'object') continue; + const mark = value as Partial; + 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(() => 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>(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(); - 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(); + 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(); + 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 }; } diff --git a/web/src/styles.css b/web/src/styles.css index 4828ce7..96ddf9e 100644 --- a/web/src/styles.css +++ b/web/src/styles.css @@ -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. */