From 2e3af19d0083be2b21d6090825e6bf74a8b3b14a Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Thu, 27 Aug 2026 12:10:32 -0400 Subject: [PATCH] Fix branch refresh. --- src/review/Server.zig | 11 ++++++++++- web/src/App.tsx | 31 +++++++++++++++++++++++++++---- web/src/api.ts | 8 +++++++- 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/src/review/Server.zig b/src/review/Server.zig index 535c36d..bc14590 100644 --- a/src/review/Server.zig +++ b/src/review/Server.zig @@ -783,7 +783,16 @@ fn handleRevision( else => "git could not produce that diff — check the base ref", }, ); - return writeJson(a, request, .ok, .{ .revision = rev }); + // The branch rides along with the digest so the page can notice that the + // work tree moved to another branch — which the digest alone cannot always + // say. `digest` hashes HEAD's *sha*, so branching off the commit you are + // already on (`git switch -c`) leaves it identical while the name in the + // review's header is now wrong. One `rev-parse` next to the `git diff` this + // endpoint already runs is not a cost worth avoiding. + return writeJson(a, request, .ok, .{ + .revision = rev, + .branch = git.currentBranch(repo, a, self.io), + }); } fn handleFile( diff --git a/web/src/App.tsx b/web/src/App.tsx index 9a712a1..87877c3 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -205,6 +205,9 @@ export default function App() { const [toast, setToast] = useState(null); const path = repo?.path ?? null; + // What the header currently says the work tree is on. The poll below compares + // against it to notice a branch switch made outside this pane. + const branch = repo?.branch ?? null; // ctxByRepo holds only the repos whose base ref you've actually changed; // anything else falls back to HEAD. Deriving rather than seeding state avoids a @@ -338,6 +341,18 @@ export default function App() { if (path) loadDiff(ctx); }, [path, ctx, loadDiff]); + // What the refresh button and the stale banner do. Deliberately more than + // reloading the patch: the reason to press either is that the world moved, and + // everything the header shows — the branch, the ref list the base picker offers, + // the draft counts — is otherwise fetched once when the page loads and never + // again. The poll below catches a branch switch on its own, but it stops once + // the diff is known to be stale, and it can't see a ref that was merely + // created; pressing refresh is the answer to both. + const reload = useCallback(() => { + loadRepo(); + loadDiff(ctx); + }, [loadRepo, loadDiff, ctx]); + // Tell the server which diff is on screen, so an agent asked to review it lands // its comments on the lines you're actually looking at. Best-effort — nothing on // screen depends on it. @@ -404,10 +419,18 @@ export default function App() { busy = true; const seq = reqRef.current; try { - const { revision: now } = await api.revision(ctx, { ignoreWhitespace: ignoreWs }); + const { revision: now, branch: on } = 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 (canceled || seq !== reqRef.current) return; + // A branch switch is news about the repository rather than about the + // diff, and the two don't always arrive together — so it is acted on + // rather than announced. The header is describing the work tree, which + // really is on another branch now; the diff below it is still the one + // you were reading, and the banner is what offers to move that. + if (on && on !== branch) loadRepo(); if (now && now !== revision) setStale(now); } catch { // A failed poll says nothing about the diff — the next one will. @@ -428,7 +451,7 @@ export default function App() { document.removeEventListener('visibilitychange', onVisible); window.removeEventListener('focus', onVisible); }; - }, [path, ctx, ignoreWs, revision, stale]); + }, [path, ctx, ignoreWs, revision, stale, branch, loadRepo]); // 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, @@ -723,7 +746,7 @@ export default function App() { diff --git a/web/src/api.ts b/web/src/api.ts index 8ec632e..975aa5c 100644 --- a/web/src/api.ts +++ b/web/src/api.ts @@ -108,13 +108,19 @@ export const api = { // `revision` that came with the diff on screen. One hash, so it's cheap to ask // for repeatedly — which is what lets the page notice the diff is out of date // without pulling it out from under whoever is reading it. + // + // `branch` is the work tree's current branch, answered here because this is the + // one thing the page asks about on a timer. The header names the branch, and + // nothing else would tell the page it had changed: the digest covers HEAD's + // sha, so branching off the commit you're on doesn't move it (see + // Server.handleRevision), and repo info is otherwise fetched once at load. revision: (ctx: DiffContext, opts: { ignoreWhitespace?: boolean } = {}) => fetch( `${apiBase}/diff/revision${q( ...ctxParams(ctx), ...(opts.ignoreWhitespace ? ['ignoreWhitespace=1'] : []), )}`, - ).then(json<{ revision: string }>), + ).then(json<{ revision: string; branch?: string }>), // Full contents of a file at a ref, for expanding collapsed context. Null when // the file doesn't exist at that ref (e.g. a newly added file).