Fix branch refresh.
This commit is contained in:
+10
-1
@@ -783,7 +783,16 @@ fn handleRevision(
|
|||||||
else => "git could not produce that diff — check the base ref",
|
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(
|
fn handleFile(
|
||||||
|
|||||||
+27
-4
@@ -205,6 +205,9 @@ export default function App() {
|
|||||||
const [toast, setToast] = useState<string | null>(null);
|
const [toast, setToast] = useState<string | null>(null);
|
||||||
|
|
||||||
const path = repo?.path ?? 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;
|
// 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
|
// 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);
|
if (path) loadDiff(ctx);
|
||||||
}, [path, ctx, loadDiff]);
|
}, [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
|
// 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
|
// its comments on the lines you're actually looking at. Best-effort — nothing on
|
||||||
// screen depends on it.
|
// screen depends on it.
|
||||||
@@ -404,10 +419,18 @@ export default function App() {
|
|||||||
busy = true;
|
busy = true;
|
||||||
const seq = reqRef.current;
|
const seq = reqRef.current;
|
||||||
try {
|
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
|
// A load that started while this was in flight has already answered the
|
||||||
// question, with a revision this closure doesn't know about.
|
// question, with a revision this closure doesn't know about.
|
||||||
if (canceled || seq !== reqRef.current) return;
|
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);
|
if (now && now !== revision) setStale(now);
|
||||||
} catch {
|
} catch {
|
||||||
// A failed poll says nothing about the diff — the next one will.
|
// A failed poll says nothing about the diff — the next one will.
|
||||||
@@ -428,7 +451,7 @@ export default function App() {
|
|||||||
document.removeEventListener('visibilitychange', onVisible);
|
document.removeEventListener('visibilitychange', onVisible);
|
||||||
window.removeEventListener('focus', 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
|
// 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,
|
// takes the newer state as the baseline — so the *next* change says so too,
|
||||||
@@ -723,7 +746,7 @@ export default function App() {
|
|||||||
|
|
||||||
<button
|
<button
|
||||||
className={`icon-btn${stale ? ' is-attention' : ''}`}
|
className={`icon-btn${stale ? ' is-attention' : ''}`}
|
||||||
onClick={() => loadDiff(ctx)}
|
onClick={reload}
|
||||||
title={stale ? 'Reload the diff — the work tree has moved on' : 'Refresh diff'}
|
title={stale ? 'Reload the diff — the work tree has moved on' : 'Refresh diff'}
|
||||||
aria-label="Refresh diff"
|
aria-label="Refresh diff"
|
||||||
>
|
>
|
||||||
@@ -779,7 +802,7 @@ export default function App() {
|
|||||||
<span className="banner-text">
|
<span className="banner-text">
|
||||||
This diff is out of date — the work tree has changed since it loaded.
|
This diff is out of date — the work tree has changed since it loaded.
|
||||||
</span>
|
</span>
|
||||||
<button className="btn-ghost" onClick={() => loadDiff(ctx)}>
|
<button className="btn-ghost" onClick={reload}>
|
||||||
<Icon name="sync" size={14} />
|
<Icon name="sync" size={14} />
|
||||||
Refresh
|
Refresh
|
||||||
</button>
|
</button>
|
||||||
|
|||||||
+7
-1
@@ -108,13 +108,19 @@ export const api = {
|
|||||||
// `revision` that came with the diff on screen. One hash, so it's cheap to ask
|
// `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
|
// 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.
|
// 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 } = {}) =>
|
revision: (ctx: DiffContext, opts: { ignoreWhitespace?: boolean } = {}) =>
|
||||||
fetch(
|
fetch(
|
||||||
`${apiBase}/diff/revision${q(
|
`${apiBase}/diff/revision${q(
|
||||||
...ctxParams(ctx),
|
...ctxParams(ctx),
|
||||||
...(opts.ignoreWhitespace ? ['ignoreWhitespace=1'] : []),
|
...(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
|
// 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).
|
// the file doesn't exist at that ref (e.g. a newly added file).
|
||||||
|
|||||||
Reference in New Issue
Block a user