diff --git a/README.md b/README.md index 2012feb..aa85f2d 100644 --- a/README.md +++ b/README.md @@ -811,6 +811,7 @@ The endpoints, all under `/t//api`: | `GET repo` | the repository, its refs, the comment counts, and the diff selection on screen | | `POST repo/context` | what the page publishes when you change the base ref | | `GET diff` | `base`, `uncommitted`, `commit`, `force`, `ignoreWhitespace` | +| `GET diff/revision` | a digest of what that same selection resolves to now — one hash, so the page can poll it | | `GET file` | a file's contents at a ref, for expanding collapsed context | | `GET/POST comments` | list, or open a thread | | `PATCH/DELETE comments/{id}` | edit or delete one | @@ -828,6 +829,25 @@ author starts as a draft. An agent has no drafting step — it posts a review it has already decided on — so `"author":"claude"` is born submitted: an open thread, with no **Submit review** click standing between it and being read. +### The diff never moves under you + +An agent editing files while you read is the normal case here, so the pane has to +have an answer for "the diff you are looking at is no longer the diff". Reloading +itself is not that answer: it would lose your scroll position, your place in a +hunk, and whatever you had half-typed into a composer. + +So the page asks `GET diff/revision` every few seconds — a hash of the patch plus +`HEAD`, which is cheap enough to ask for on a timer and catches both an +uncommitted edit and work being committed out from under the range. When it stops +matching the revision the diff came with, a banner says so and offers the refresh. +Dismissing it keeps the diff you are reading and re-arms against what is there +now, so the *next* change tells you too. Polling stops while the pane is hidden, +and starts again the moment it comes back. + +Comments are the other half, and they work the other way round: those arrive over +`GET events` and are applied live, because a thread appearing in the rail doesn't +move anything you were reading. + ### Comments are markdown A comment body is markdown, rendered where it is read: fenced code (highlighted diff --git a/src/review/Server.zig b/src/review/Server.zig index bd747d3..535c36d 100644 --- a/src/review/Server.zig +++ b/src/review/Server.zig @@ -608,6 +608,9 @@ fn handleApi( if (std.mem.eql(u8, path, "diff")) { return self.handleDiff(a, request, repo, tab, query); } + if (std.mem.eql(u8, path, "diff/revision")) { + return self.handleRevision(a, request, repo, tab, query); + } if (std.mem.eql(u8, path, "file")) { return self.handleFile(a, request, repo, query); } @@ -709,6 +712,21 @@ fn clampCopy(buf: []u8, value: []const u8) []const u8 { return buf[0..n]; } +/// The diff selection a request names, falling back to the one the tab last +/// published for anything it leaves out. +fn queryContext( + a: std.mem.Allocator, + query: []const u8, + tab: Resolved, +) !model.DiffContext { + return .{ + .base = try queryValue(a, query, "base") orelse + (if (tab.ctx) |c| c.base else "HEAD"), + .uncommitted = queryFlag(a, query, "uncommitted") catch false, + .commit = try queryValue(a, query, "commit") orelse "", + }; +} + fn handleDiff( self: *Server, a: std.mem.Allocator, @@ -717,12 +735,7 @@ fn handleDiff( tab: Resolved, query: []const u8, ) !void { - const ctx: model.DiffContext = .{ - .base = try queryValue(a, query, "base") orelse - (if (tab.ctx) |c| c.base else "HEAD"), - .uncommitted = queryFlag(a, query, "uncommitted") catch false, - .commit = try queryValue(a, query, "commit") orelse "", - }; + const ctx = try queryContext(a, query, tab); const opts: git.Options = .{ .force = queryFlag(a, query, "force") catch false, .ignore_whitespace = queryFlag(a, query, "ignoreWhitespace") catch false, @@ -740,6 +753,39 @@ fn handleDiff( return writeJson(a, request, .ok, payload); } +/// A digest of what the selection resolves to *now*, for a page holding the one +/// that came with the diff it is showing. Answering "has this moved?" without +/// shipping the patch again is the whole reason it exists: the page polls this +/// and raises a banner, rather than reloading the diff under whoever is reading +/// it. See `git.revision`. +fn handleRevision( + self: *Server, + a: std.mem.Allocator, + request: *std.http.Server.Request, + repo: git.Repo, + tab: Resolved, + query: []const u8, +) !void { + const ctx = try queryContext(a, query, tab); + const opts: git.Options = .{ + // Forced, because the answer has to be comparable with the revision the + // page already has — which, if it got here, is one it was given. + .force = true, + .ignore_whitespace = queryFlag(a, query, "ignoreWhitespace") catch false, + }; + + const rev = git.revision(repo, a, self.io, ctx, opts) catch |err| return writeError( + a, + request, + if (err == error.BadCommit) .bad_request else .bad_gateway, + switch (err) { + error.BadCommit => "commit is not a sha", + else => "git could not produce that diff — check the base ref", + }, + ); + return writeJson(a, request, .ok, .{ .revision = rev }); +} + fn handleFile( self: *Server, a: std.mem.Allocator, diff --git a/src/review/git.zig b/src/review/git.zig index 373cc94..b6cb805 100644 --- a/src/review/git.zig +++ b/src/review/git.zig @@ -339,9 +339,61 @@ pub fn diff( .files = files, .commits = listed.items, .moreCommits = listed.more, + .revision = try digest(repo, gpa, io, patch), }; } +/// A digest of what a selection resolves to right now, for noticing that the +/// diff already on someone's screen has been overtaken. +/// +/// Cheap enough to ask for on a timer — the answer is one hash — which is the +/// point: a review pane that reloaded itself every time an agent saved a file +/// would move the diff out from under whoever is reading it, so the page polls +/// this instead and says so. +/// +/// The oversize guard is deliberately not applied. This is asked for by a page +/// that already has a patch on screen, so the answer has to be comparable with +/// the `revision` that came with it, guard or no guard. +pub fn revision( + repo: Repo, + gpa: std.mem.Allocator, + io: std.Io, + ctx: model.DiffContext, + opts: Options, +) Error![]const u8 { + try validateCommit(ctx.commit); + const patch = try run(gpa, io, repo.path, try diffArgs(gpa, ctx, opts, &.{ + "--no-color", + "--find-renames", + })); + return digest(repo, gpa, io, patch); +} + +/// The digest itself, given a patch already in hand: the patch, hashed, plus +/// HEAD. +/// +/// Both halves are needed. The patch alone misses work being *committed*, which +/// leaves `base`..worktree byte for byte the same while the commit list beside +/// it grows an entry. HEAD alone misses every edit that hasn't been committed, +/// which is most of what a review watches for. +/// +/// Nothing here is adversarial — a collision costs one banner that never +/// appears — so a 64-bit non-cryptographic hash is plenty. +fn digest( + repo: Repo, + gpa: std.mem.Allocator, + io: std.Io, + patch: []const u8, +) Error![]const u8 { + // Best-effort: a repository with no commits has no HEAD to resolve, and the + // patch on its own is still a usable digest. + const head = run(gpa, io, repo.path, &.{ "rev-parse", "HEAD" }) catch ""; + var hasher = std.hash.Wyhash.init(0); + hasher.update(trim(head)); + hasher.update(patch); + return std.fmt.allocPrint(gpa, "{x}", .{hasher.final()}); +} + /// Whether a change set is past what the UI can render at once. Binary files /// count for no lines, hence the file cap alongside the line one. fn oversized(files: []const model.DiffFile) bool { diff --git a/src/review/model.zig b/src/review/model.zig index 9794d0f..3f5d331 100644 --- a/src/review/model.zig +++ b/src/review/model.zig @@ -137,6 +137,15 @@ pub const DiffPayload = struct { /// `files` is still filled in, so the caller can say how big it is and /// offer to load it anyway with `force`. oversized: bool = false, + + /// A digest of what this selection resolved to when the patch was produced + /// — see `git.revision`. The page keeps it and asks `GET api/diff/revision` + /// for the current one every so often, which is how it can say the diff on + /// screen has gone stale without reloading it under the reader. + /// + /// Empty when the patch was withheld, since there is then nothing on screen + /// to go stale. + revision: []const u8 = "", }; /// What `GET api/repo` returns for a tab that has a review open. diff --git a/web/src/App.tsx b/web/src/App.tsx index 97f3110..bbe19a1 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -13,6 +13,7 @@ import { ConfirmDialog } from './components/ConfirmDialog'; import { DiffView } from './components/DiffView'; import { Icon } from './components/Icon'; import { FileList } from './components/FileList'; +import { Menu, MenuItem, MenuSeparator } from './components/Menu'; import { OutdatedPanel } from './components/Outdated'; import { OversizeNotice, OversizeWarning } from './components/Oversize'; import { Resizer } from './components/Resizer'; @@ -43,6 +44,20 @@ const COMMENTS_MAX = 720; // review in a cramped pane must not cost you the rail sizes you chose. const MIN_DIFF = 420; +// How often the pane asks whether the diff it is showing still matches the work +// tree, and why it asks at all rather than being told. +// +// The pane must not reload itself. An agent editing files while you read is the +// normal case here, and a diff that reloads under you loses your scroll position, +// your place in a hunk, and — if the composer is open — what you were typing. So +// the page polls a digest (see api.revision) and puts a banner up; refreshing +// stays your decision. +// +// A poll is a `git diff` on the server, so it isn't free. At this interval it is +// invisible next to the work of whatever is doing the editing, and it stops +// entirely while the pane is hidden or already known to be stale. +const POLL_MS = 8000; + const CTX_KEY = 'review-ctx-by-repo'; const IGNORE_WS_KEY = 'review-ignore-whitespace'; @@ -177,7 +192,11 @@ export default function App() { const [commentsOpen, setCommentsOpen] = useState( () => localStorage.getItem('review-comments-open') !== 'false', ); - const [connected, setConnected] = useState(false); + // The digest of the diff on screen, and a newer one a poll has seen — see + // POLL_MS. `stale` holding the newer revision rather than a bare flag is what + // lets "not now" re-arm against the current state instead of going quiet. + const [revision, setRevision] = useState(null); + const [stale, setStale] = useState(null); // Tracked so the rail clamping below re-runs when the pane is resized — // dragging a split in playpen is the common case, not a rare one. const [viewport, setViewport] = useState(() => window.innerWidth); @@ -296,6 +315,8 @@ export default function App() { if (seq !== reqRef.current) return; setComments(cs); setError(null); + setStale(null); + setRevision(d.revision || null); if (d.oversized) { setPayload(null); setOversized(d); @@ -354,10 +375,13 @@ export default function App() { // Live updates. Every event on this stream is about this review — the stream is // the tab's own — so anything that isn't the opening handshake means the comment // list moved and is worth refetching. + // + // Comments only. The diff is not pushed: the server has no watcher on the work + // tree, and a comment arriving is a change to the rail, which can be applied + // under the reader safely. A changed *diff* cannot — see the poll below. useSSE( useCallback( (e) => { - setConnected(true); if (e.type === 'connected') return; refetchComments(); }, @@ -365,6 +389,56 @@ export default function App() { ), ); + // Notice that the diff on screen has been overtaken, and say so. Nothing here + // reloads anything: it sets `stale`, the banner offers the refresh, and the + // polling stops until one or the other happens. + useEffect(() => { + if (!path || !revision || stale) return; + + let cancelled = false; + let busy = false; + + const check = async () => { + // A hidden pane is a pane nobody is reading. It gets checked the moment it + // comes back instead, which is when the answer matters. + if (cancelled || busy || document.hidden) return; + busy = true; + const seq = reqRef.current; + try { + const { revision: now } = await api.revision(ctx, { ignoreWhitespace: ignoreWs }); + // A load that started while this was in flight has already answered the + // question, with a revision this closure doesn't know about. + if (cancelled || seq !== reqRef.current) return; + if (now && now !== revision) setStale(now); + } catch { + // A failed poll says nothing about the diff — the next one will. + } finally { + busy = false; + } + }; + + const timer = window.setInterval(check, POLL_MS); + const onVisible = () => { + if (!document.hidden) check(); + }; + document.addEventListener('visibilitychange', onVisible); + window.addEventListener('focus', onVisible); + return () => { + cancelled = true; + window.clearInterval(timer); + document.removeEventListener('visibilitychange', onVisible); + window.removeEventListener('focus', onVisible); + }; + }, [path, ctx, ignoreWs, revision, stale]); + + // dismissStale keeps the diff you are reading and stops pointing at it, but + // takes the newer state as the baseline — so the *next* change says so too, + // rather than the banner being a one-off you can only silence once. + const dismissStale = useCallback(() => { + setRevision((prev) => stale ?? prev); + setStale(null); + }, [stale]); + // submitDraft creates the comment for the currently-open draft (line/range, // file, or review level). const submitDraft = useCallback( @@ -546,22 +620,31 @@ export default function App() { return (
+ {/* One row, and it has to survive a pane a third of a screen wide. What + earns a place on it is what you reach for *while reading* a diff; the + rest is in the overflow menu, and every label on it is a `.btn-label` + the narrow layout can drop without the control going with it. */}
-
+
{repo.path.split('/').pop()} - - {repo.branch} - + {repo.branch}