Improve toolbar review layout.
This commit is contained in:
Greyson Parrelli
+155
-41
@@ -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<string | null>(null);
|
||||
const [stale, setStale] = useState<string | null>(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 (
|
||||
<div className="app">
|
||||
{/* 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. */}
|
||||
<header className="topbar">
|
||||
<div className="brand">
|
||||
<div className="brand" title={`${repo.path} · ${repo.branch}`}>
|
||||
<span className="brand-mark">
|
||||
<Icon name="file-diff" />
|
||||
</span>
|
||||
<span className="brand-name">{repo.path.split('/').pop()}</span>
|
||||
<span className="brand-branch" title={repo.path}>
|
||||
{repo.branch}
|
||||
</span>
|
||||
<span className="brand-branch">{repo.branch}</span>
|
||||
</div>
|
||||
|
||||
<div className="controls">
|
||||
<label className="control">
|
||||
<span className="control-label">base</span>
|
||||
{/* Capped in CSS, not left to size itself: a native select is as wide
|
||||
as its widest option, and a repository with a few hundred refs in
|
||||
it — one `origin/feature/…` is enough — makes that the whole bar.
|
||||
The name on screen is short; the list opens at full width. */}
|
||||
<select
|
||||
className="base-select"
|
||||
value={ctx.base}
|
||||
aria-label="Base ref"
|
||||
title={`Diffing against ${ctx.base}`}
|
||||
// A commit selected out of the old range has no place in the new
|
||||
// one, so changing the base drops back to the whole change set.
|
||||
onChange={(e) => setCtx({ ...ctx, base: e.target.value, commit: undefined })}
|
||||
@@ -590,13 +673,15 @@ export default function App() {
|
||||
className={`toggle${ctx.uncommitted && !ctx.commit ? ' is-on' : ''}`}
|
||||
onClick={() => setCtx({ ...ctx, uncommitted: !ctx.uncommitted })}
|
||||
disabled={!!ctx.commit}
|
||||
aria-pressed={ctx.uncommitted && !ctx.commit}
|
||||
title={
|
||||
ctx.commit
|
||||
? "Doesn't apply while you're reading a single commit"
|
||||
: 'Include uncommitted working-tree changes'
|
||||
}
|
||||
>
|
||||
uncommitted
|
||||
<Icon name="pencil" size={14} />
|
||||
<span className="btn-label">uncommitted</span>
|
||||
</button>
|
||||
|
||||
{/* Which commit is on screen, and the way back out of it. */}
|
||||
@@ -612,15 +697,6 @@ export default function App() {
|
||||
</button>
|
||||
)}
|
||||
|
||||
<button
|
||||
className={`toggle${ignoreWs ? ' is-on' : ''}`}
|
||||
onClick={() => setIgnoreWs(!ignoreWs)}
|
||||
aria-pressed={ignoreWs}
|
||||
title="Ignore whitespace-only changes (git diff -w) — files with nothing else in them leave the change set"
|
||||
>
|
||||
ignore whitespace
|
||||
</button>
|
||||
|
||||
<div className="segmented" role="group" aria-label="Diff layout">
|
||||
<button
|
||||
className={viewType === 'split' ? 'is-active' : ''}
|
||||
@@ -628,7 +704,8 @@ export default function App() {
|
||||
aria-pressed={viewType === 'split'}
|
||||
title="Split view"
|
||||
>
|
||||
<Icon name="columns" size={14} /> split
|
||||
<Icon name="columns" size={14} />
|
||||
<span className="btn-label">split</span>
|
||||
</button>
|
||||
<button
|
||||
className={viewType === 'unified' ? 'is-active' : ''}
|
||||
@@ -636,7 +713,8 @@ export default function App() {
|
||||
aria-pressed={viewType === 'unified'}
|
||||
title="Unified view"
|
||||
>
|
||||
<Icon name="rows" size={14} /> unified
|
||||
<Icon name="rows" size={14} />
|
||||
<span className="btn-label">unified</span>
|
||||
</button>
|
||||
</div>
|
||||
|
||||
@@ -645,42 +723,78 @@ export default function App() {
|
||||
{payload && <ReviewProgress files={payload.files} viewed={viewed} />}
|
||||
|
||||
<button
|
||||
className="icon-btn"
|
||||
className={`icon-btn${stale ? ' is-attention' : ''}`}
|
||||
onClick={() => loadDiff(ctx)}
|
||||
title="Refresh diff"
|
||||
title={stale ? 'Reload the diff — the work tree has moved on' : 'Refresh diff'}
|
||||
aria-label="Refresh diff"
|
||||
>
|
||||
<Icon name="sync" />
|
||||
</button>
|
||||
<button
|
||||
className="icon-btn"
|
||||
onClick={() => setTheme(theme === 'dark' ? 'light' : 'dark')}
|
||||
title="Toggle theme"
|
||||
aria-label="Toggle theme"
|
||||
>
|
||||
<Icon name={theme === 'dark' ? 'moon' : 'sun'} />
|
||||
</button>
|
||||
<button
|
||||
className="icon-btn is-danger"
|
||||
onClick={() => setResetOpen(true)}
|
||||
title="Reset review — delete every comment and clear viewed files"
|
||||
aria-label="Reset review"
|
||||
>
|
||||
<Icon name="trash" />
|
||||
</button>
|
||||
|
||||
<span className={`conn${connected ? ' is-live' : ''}`} title="Live connection">
|
||||
<Icon name="dot-fill" size={12} />
|
||||
{connected ? 'live' : 'offline'}
|
||||
</span>
|
||||
{/* Set-and-forget controls. `marked` puts a dot on the trigger when one
|
||||
of them is not on its default, so a diff that is quietly hiding
|
||||
whitespace never looks like a diff that has none. */}
|
||||
<Menu icon="kebab-horizontal" label="Review options" marked={ignoreWs}>
|
||||
{(close) => (
|
||||
<>
|
||||
<MenuItem
|
||||
checked={ignoreWs}
|
||||
onClick={() => setIgnoreWs(!ignoreWs)}
|
||||
hint="git diff -w — files with nothing but whitespace in them leave the change set"
|
||||
>
|
||||
Ignore whitespace
|
||||
</MenuItem>
|
||||
<MenuItem
|
||||
icon={theme === 'dark' ? 'sun' : 'moon'}
|
||||
onClick={() => setTheme(theme === 'dark' ? 'light' : 'dark')}
|
||||
>
|
||||
{theme === 'dark' ? 'Light theme' : 'Dark theme'}
|
||||
</MenuItem>
|
||||
<MenuSeparator />
|
||||
<MenuItem
|
||||
icon="trash"
|
||||
danger
|
||||
onClick={() => {
|
||||
close();
|
||||
setResetOpen(true);
|
||||
}}
|
||||
hint="Delete every comment and clear the viewed files"
|
||||
>
|
||||
Reset review…
|
||||
</MenuItem>
|
||||
</>
|
||||
)}
|
||||
</Menu>
|
||||
|
||||
<button className="btn-submit" onClick={submitReview} disabled={draftCount === 0}>
|
||||
Submit review
|
||||
Submit<span className="btn-label-tail"> review</span>
|
||||
{draftCount > 0 && <span className="count">{draftCount}</span>}
|
||||
</button>
|
||||
</div>
|
||||
</header>
|
||||
|
||||
{/* The diff has moved on. Said rather than acted on: see POLL_MS. */}
|
||||
{stale && (
|
||||
<div className="banner banner-stale">
|
||||
<Icon name="alert" />
|
||||
<span className="banner-text">
|
||||
This diff is out of date — the work tree has changed since it loaded.
|
||||
</span>
|
||||
<button className="btn-ghost" onClick={() => loadDiff(ctx)}>
|
||||
<Icon name="sync" size={14} />
|
||||
Refresh
|
||||
</button>
|
||||
<button
|
||||
className="icon-btn"
|
||||
onClick={dismissStale}
|
||||
title="Keep reading this one — you'll be told again if it changes further"
|
||||
aria-label="Dismiss"
|
||||
>
|
||||
<Icon name="x" />
|
||||
</button>
|
||||
</div>
|
||||
)}
|
||||
|
||||
{error && (
|
||||
<div className="banner banner-error">
|
||||
<Icon name="alert" />
|
||||
|
||||
Reference in New Issue
Block a user