diff --git a/packages/cli/src/agent.ts b/packages/cli/src/agent.ts index fe984b1..b9d6cd3 100644 --- a/packages/cli/src/agent.ts +++ b/packages/cli/src/agent.ts @@ -15,6 +15,7 @@ import { } from './threads.js'; import { createTour, addTourStep, updateTourStatus } from './tours.js'; import { readAnchor } from './anchor.js'; +import { startReviewRun, finishReviewRun } from './review-run.js'; import { readRepoConfig, DEFAULT_SEVERITIES, resolveInRepo, REPO_CONFIG_FILE } from '@diffity/git'; import { readFileSync } from 'node:fs'; @@ -243,6 +244,25 @@ Examples: process.stdout.write(raw); }); + agent + .command('review-start') + .description('Announce that a review is under way, so the page can say so') + .option('--note ', 'What is being reviewed', '') + .action((opts) => { + const session = requireSession(); + startReviewRun(session.id, opts.note); + console.log(pc.green('Review marked as in progress')); + }); + + agent + .command('review-done') + .description('Announce that the review is finished') + .action(() => { + const session = requireSession(); + finishReviewRun(session.id); + console.log(pc.green('Review marked as finished')); + }); + agent .command('standards') .description("Print the project's review standards and severity labels") diff --git a/packages/cli/src/db.ts b/packages/cli/src/db.ts index adcad7e..df3a26d 100644 --- a/packages/cli/src/db.ts +++ b/packages/cli/src/db.ts @@ -113,6 +113,13 @@ function migrateDb(db: DatabaseSync): void { created_at TEXT NOT NULL DEFAULT (datetime('now')) ); + CREATE TABLE IF NOT EXISTS review_runs ( + session_id TEXT PRIMARY KEY REFERENCES review_sessions(id), + started_at TEXT NOT NULL, + finished_at TEXT, + note TEXT NOT NULL DEFAULT '' + ); + CREATE INDEX IF NOT EXISTS idx_tours_session ON tours(session_id); CREATE INDEX IF NOT EXISTS idx_tour_steps_tour ON tour_steps(tour_id); `); diff --git a/packages/cli/src/fingerprint.ts b/packages/cli/src/fingerprint.ts new file mode 100644 index 0000000..834d2db --- /dev/null +++ b/packages/cli/src/fingerprint.ts @@ -0,0 +1,26 @@ +import { createHash } from 'node:crypto'; +import { getDiffStat, getDiffStatForRef, getHeadHash, getUntrackedFiles } from '@diffity/git'; + +/** + * A diffstat only counts lines, so a commit that rewrites the same number of them produces an + * identical stat and the diff would look unchanged. Including the head commit makes any new + * commit visible, while the stat still catches edits to the working tree, where HEAD does not move. + */ +export function computeDiffFingerprint( + ref: string | null, + diffArgs: string[] = [], + includeUntracked = false, +): string { + let stat: string; + + if (ref) { + stat = getDiffStatForRef(ref); + } else { + stat = getDiffStat(diffArgs); + if (includeUntracked) { + stat += '\n' + getUntrackedFiles().join('\n'); + } + } + + return createHash('sha1').update(`${getHeadHash()}\n${stat}`).digest('hex'); +} diff --git a/packages/cli/src/review-routes.ts b/packages/cli/src/review-routes.ts index dc7e041..3883133 100644 --- a/packages/cli/src/review-routes.ts +++ b/packages/cli/src/review-routes.ts @@ -11,7 +11,7 @@ import { type ThreadAuthor, type ThreadStatus, } from './threads.js'; -import { getCurrentSession } from './session.js'; +import { getCurrentSession, resolveSessionId } from './session.js'; import { sendJson, sendError, withJsonBody } from './http-utils.js'; export function handleReviewRoute(req: IncomingMessage, res: ServerResponse, pathname: string, url: URL): boolean { @@ -22,9 +22,9 @@ export function handleReviewRoute(req: IncomingMessage, res: ServerResponse, pat } if (pathname === '/api/threads' && req.method === 'GET') { - const sid = url.searchParams.get('session'); + const sid = resolveSessionId(url.searchParams.get('session')); if (!sid) { - sendError(res, 400, 'Missing session parameter'); + sendError(res, 400, 'No review session'); return true; } const status = url.searchParams.get('status') as ThreadStatus | null; diff --git a/packages/cli/src/review-run.ts b/packages/cli/src/review-run.ts new file mode 100644 index 0000000..95071f3 --- /dev/null +++ b/packages/cli/src/review-run.ts @@ -0,0 +1,69 @@ +import { getDb, queryOne } from './db.js'; + +export interface ReviewRun { + inProgress: boolean; + startedAt: string | null; + note: string; +} + +const ABSENT: ReviewRun = { inProgress: false, startedAt: null, note: '' }; + +/** + * Whether an agent is part-way through writing findings. A reader who cannot tell the difference + * between "no problems found" and "not finished looking" can approve a change too early. + */ +export function getReviewRun(sessionId: string): ReviewRun { + const row = queryOne<{ started_at: string; finished_at: string | null; note: string }>( + 'SELECT started_at, finished_at, note FROM review_runs WHERE session_id = ?', + sessionId, + ); + + if (!row) { + return ABSENT; + } + + return { + inProgress: !row.finished_at, + startedAt: row.started_at, + note: row.note, + }; +} + +export function startReviewRun(sessionId: string, note: string): void { + getDb() + .prepare( + `INSERT INTO review_runs (session_id, started_at, finished_at, note) + VALUES (?, datetime('now'), NULL, ?) + ON CONFLICT(session_id) DO UPDATE SET + started_at = datetime('now'), + finished_at = NULL, + note = excluded.note`, + ) + .run(sessionId, note); +} + +export function finishReviewRun(sessionId: string): void { + getDb() + .prepare("UPDATE review_runs SET finished_at = datetime('now') WHERE session_id = ?") + .run(sessionId); +} + +/** An unfinished run follows the session, so committing mid-review does not clear the warning. */ +export function carryReviewRun(fromSessionId: string, toSessionId: string): void { + const run = queryOne<{ started_at: string; note: string }>( + 'SELECT started_at, note FROM review_runs WHERE session_id = ? AND finished_at IS NULL', + fromSessionId, + ); + + if (!run) { + return; + } + + getDb() + .prepare( + `INSERT INTO review_runs (session_id, started_at, finished_at, note) + VALUES (?, ?, NULL, ?) + ON CONFLICT(session_id) DO UPDATE SET started_at = excluded.started_at, finished_at = NULL, note = excluded.note`, + ) + .run(toSessionId, run.started_at, run.note); +} diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index 433f298..fce985f 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -12,7 +12,6 @@ import { dirname } from 'node:path'; import { parseDiff, type ParsedDiff } from '@diffity/parser'; import { getDiff, - getDiffStatForRef, getDiffStat, getUntrackedFiles, getUntrackedDiff, @@ -47,6 +46,8 @@ import { type ReviewEvent, } from '@diffity/github'; import { findOrCreateSession } from './session.js'; +import { computeDiffFingerprint } from './fingerprint.js'; +import { getReviewRun } from './review-run.js'; import { createThread, addReply, getThreadsForSession } from './threads.js'; import { handleReviewRoute } from './review-routes.js'; import { handleTourRoute } from './tour-routes.js'; @@ -397,20 +398,9 @@ export function startServer(options: ServerOptions): Promise { if (pathname === '/api/diff-fingerprint') { const ref = url.searchParams.get('ref'); - let stat: string; - if (ref) { - stat = getDiffStatForRef(ref); - } else { - stat = getDiffStat(diffArgs); - if (includeUntracked) { - stat += '\n' + getUntrackedFiles().join('\n'); - } - } - const hash = createHash('sha1') - .update(stat) - .digest('hex') - .slice(0, 12); - sendJson(res, { fingerprint: hash }); + sendJson(res, { + fingerprint: computeDiffFingerprint(ref, diffArgs, includeUntracked), + }); return; } @@ -486,6 +476,7 @@ export function startServer(options: ServerOptions): Promise { description: refDescription, capabilities, sessionId, + review: sessionId ? getReviewRun(sessionId) : null, github: githubRemote, editor: editorAvailable, }); diff --git a/packages/cli/src/session.ts b/packages/cli/src/session.ts index a0933ed..2f088ef 100644 --- a/packages/cli/src/session.ts +++ b/packages/cli/src/session.ts @@ -4,6 +4,7 @@ import { join } from 'node:path'; import { getHeadHash, getDiffityDir } from '@diffity/git'; import { getDb, queryAll, queryOne } from './db.js'; import { reanchorInWorkingTree } from './anchor.js'; +import { carryReviewRun } from './review-run.js'; import { updateThreadLines } from './threads.js'; export interface Session { @@ -74,6 +75,7 @@ export function carryForward(fromSessionId: string, toSessionId: string): void { fromSessionId, ); + carryReviewRun(fromSessionId, toSessionId); reanchorThreads(toSessionId); } @@ -101,6 +103,32 @@ function reanchorThreads(sessionId: string): void { } } +/** + * The session a request is about. A browser tab holds whichever id it loaded with, and a commit + * since then will have carried the threads into a newer session for the same ref — so honouring a + * stale id literally would tell the tab the review is empty. + */ +export function resolveSessionId(sessionId: string | null | undefined): string { + if (!sessionId) { + return getCurrentSession()?.id ?? ''; + } + + const known = queryOne<{ ref: string }>( + 'SELECT ref FROM review_sessions WHERE id = ?', + sessionId, + ); + if (!known) { + return sessionId; + } + + const newest = queryOne<{ id: string }>( + 'SELECT id FROM review_sessions WHERE ref = ? ORDER BY created_at DESC, rowid DESC LIMIT 1', + known.ref, + ); + + return newest?.id ?? sessionId; +} + export function getCurrentSession(): Session | null { try { const raw = readFileSync(sessionFilePath(), 'utf-8'); diff --git a/packages/cli/src/tour-routes.ts b/packages/cli/src/tour-routes.ts index 82b27af..c36a369 100644 --- a/packages/cli/src/tour-routes.ts +++ b/packages/cli/src/tour-routes.ts @@ -8,12 +8,13 @@ import { type TourStatus, } from './tours.js'; import { sendJson, sendError, withJsonBody } from './http-utils.js'; +import { resolveSessionId } from './session.js'; export function handleTourRoute(req: IncomingMessage, res: ServerResponse, pathname: string, url: URL): boolean { if (pathname === '/api/tours' && req.method === 'GET') { - const sid = url.searchParams.get('session'); + const sid = resolveSessionId(url.searchParams.get('session')); if (!sid) { - sendError(res, 400, 'Missing session parameter'); + sendError(res, 400, 'No review session'); return true; } const tours = getToursForSession(sid); diff --git a/packages/cli/tests/review-run.test.ts b/packages/cli/tests/review-run.test.ts new file mode 100644 index 0000000..1702c18 --- /dev/null +++ b/packages/cli/tests/review-run.test.ts @@ -0,0 +1,105 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +let root: string; +let repoDir: string; +let origCwd: string; + +function commit(name: string): void { + writeFileSync(join(repoDir, name), `${name}\n`); + execFileSync('git', ['add', '.'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', name], { cwd: repoDir, stdio: 'pipe' }); +} + +beforeAll(() => { + origCwd = process.cwd(); + root = mkdtempSync(join(tmpdir(), 'diffity-run-')); + repoDir = join(root, 'repo'); + execFileSync('git', ['init', '-b', 'main', repoDir], { stdio: 'pipe' }); + execFileSync('git', ['config', 'user.email', 't@t'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.name', 'T'], { cwd: repoDir, stdio: 'pipe' }); + commit('a.txt'); + process.env.DIFFITY_DATA_DIR = join(root, 'notes'); + process.chdir(repoDir); +}); + +afterAll(() => { + process.chdir(origCwd); + delete process.env.DIFFITY_DATA_DIR; + rmSync(root, { recursive: true, force: true }); +}); + +describe('a review run', () => { + it('is absent until one starts', async () => { + const { getReviewRun } = await import('../src/review-run.js'); + const { findOrCreateSession } = await import('../src/session.js'); + + expect(getReviewRun(findOrCreateSession('work').id).inProgress).toBe(false); + }); + + it('is in progress once started, carrying its note', async () => { + const { getReviewRun, startReviewRun } = await import('../src/review-run.js'); + const { findOrCreateSession } = await import('../src/session.js'); + const session = findOrCreateSession('work'); + + startReviewRun(session.id, 'reading the idempotency change'); + const run = getReviewRun(session.id); + + expect(run.inProgress).toBe(true); + expect(run.note).toBe('reading the idempotency change'); + expect(run.startedAt).toBeTruthy(); + }); + + it('stops being in progress when it finishes', async () => { + const { getReviewRun, startReviewRun, finishReviewRun } = await import('../src/review-run.js'); + const { findOrCreateSession } = await import('../src/session.js'); + const session = findOrCreateSession('work'); + + startReviewRun(session.id, ''); + finishReviewRun(session.id); + + expect(getReviewRun(session.id).inProgress).toBe(false); + }); + + it('keeps one run per session when started twice', async () => { + const { getReviewRun, startReviewRun } = await import('../src/review-run.js'); + const { findOrCreateSession } = await import('../src/session.js'); + const session = findOrCreateSession('work'); + + startReviewRun(session.id, 'first'); + startReviewRun(session.id, 'second'); + + expect(getReviewRun(session.id).note).toBe('second'); + expect(getReviewRun(session.id).inProgress).toBe(true); + }); + + it('follows the session when a commit moves HEAD mid-review', async () => { + const { getReviewRun, startReviewRun } = await import('../src/review-run.js'); + const { findOrCreateSession } = await import('../src/session.js'); + const before = findOrCreateSession('work'); + startReviewRun(before.id, 'still going'); + + commit('b.txt'); + const after = findOrCreateSession('work'); + + expect(after.id).not.toBe(before.id); + expect(getReviewRun(after.id).inProgress).toBe(true); + expect(getReviewRun(after.id).note).toBe('still going'); + }); + + it('does not resurrect a finished run on a later session', async () => { + const { getReviewRun, startReviewRun, finishReviewRun } = await import('../src/review-run.js'); + const { findOrCreateSession } = await import('../src/session.js'); + const before = findOrCreateSession('work'); + startReviewRun(before.id, 'done with this'); + finishReviewRun(before.id); + + commit('c.txt'); + const after = findOrCreateSession('work'); + + expect(getReviewRun(after.id).inProgress).toBe(false); + }); +}); diff --git a/packages/cli/tests/session-fallback.test.ts b/packages/cli/tests/session-fallback.test.ts new file mode 100644 index 0000000..c432e8f --- /dev/null +++ b/packages/cli/tests/session-fallback.test.ts @@ -0,0 +1,60 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +let root: string; +let repoDir: string; +let origCwd: string; + +beforeAll(() => { + origCwd = process.cwd(); + root = mkdtempSync(join(tmpdir(), 'diffity-fallback-')); + repoDir = join(root, 'repo'); + execFileSync('git', ['init', '-b', 'main', repoDir], { stdio: 'pipe' }); + execFileSync('git', ['config', 'user.email', 't@t'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.name', 'T'], { cwd: repoDir, stdio: 'pipe' }); + writeFileSync(join(repoDir, 'a.txt'), 'a\n'); + execFileSync('git', ['add', '.'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'init'], { cwd: repoDir, stdio: 'pipe' }); + process.env.DIFFITY_DATA_DIR = join(root, 'notes'); + process.chdir(repoDir); +}); + +afterAll(() => { + process.chdir(origCwd); + delete process.env.DIFFITY_DATA_DIR; + rmSync(root, { recursive: true, force: true }); +}); + +describe('resolveSessionId', () => { + it('falls back to the current session when the client names none', async () => { + const { findOrCreateSession, resolveSessionId } = await import('../src/session.js'); + const session = findOrCreateSession('work'); + + expect(resolveSessionId(null)).toBe(session.id); + expect(resolveSessionId(undefined)).toBe(session.id); + }); + + it('follows a superseded session to the one that took its threads', async () => { + const { findOrCreateSession, resolveSessionId, carryForward } = await import('../src/session.js'); + const old = findOrCreateSession('work'); + + writeFileSync(join(repoDir, 'b.txt'), 'b\n'); + execFileSync('git', ['add', '.'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'move head'], { cwd: repoDir, stdio: 'pipe' }); + const fresh = findOrCreateSession('work'); + + expect(fresh.id).not.toBe(old.id); + // A browser tab still holding the old id must not be told the review is empty. + expect(resolveSessionId(old.id)).toBe(fresh.id); + expect(carryForward).toBeTypeOf('function'); + }); + + it('leaves an unknown session alone rather than inventing one', async () => { + const { resolveSessionId } = await import('../src/session.js'); + + expect(resolveSessionId('not-a-session')).toBe('not-a-session'); + }); +}); diff --git a/packages/cli/tests/staleness.test.ts b/packages/cli/tests/staleness.test.ts new file mode 100644 index 0000000..7776d91 --- /dev/null +++ b/packages/cli/tests/staleness.test.ts @@ -0,0 +1,59 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +let root: string; +let repoDir: string; +let origCwd: string; + +function git(args: string[]): string { + return execFileSync('git', args, { cwd: repoDir, encoding: 'utf-8', stdio: 'pipe' }).trim(); +} + +function commit(name: string, content: string): void { + writeFileSync(join(repoDir, name), content); + git(['add', '.']); + git(['commit', '-m', name]); +} + +beforeAll(() => { + origCwd = process.cwd(); + root = mkdtempSync(join(tmpdir(), 'diffity-stale-')); + repoDir = join(root, 'repo'); + execFileSync('git', ['init', '-b', 'main', repoDir], { stdio: 'pipe' }); + execFileSync('git', ['config', 'user.email', 't@t'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.name', 'T'], { cwd: repoDir, stdio: 'pipe' }); + commit('base.txt', 'base\n'); + git(['tag', 'start']); + process.env.DIFFITY_DATA_DIR = join(root, 'notes'); + process.chdir(repoDir); +}); + +afterAll(() => { + process.chdir(origCwd); + delete process.env.DIFFITY_DATA_DIR; + rmSync(root, { recursive: true, force: true }); +}); + +describe('diff fingerprint', () => { + it('changes when a commit rewrites the same number of lines', async () => { + const { computeDiffFingerprint } = await import('../src/fingerprint.js'); + + commit('a.txt', 'one\ntwo\n'); + const before = computeDiffFingerprint('start'); + + // Same line counts, different content — a diffstat alone cannot tell these apart. + commit('a.txt', 'one\nTWO\n'); + const after = computeDiffFingerprint('start'); + + expect(after).not.toBe(before); + }); + + it('is stable when nothing changed', async () => { + const { computeDiffFingerprint } = await import('../src/fingerprint.js'); + + expect(computeDiffFingerprint('start')).toBe(computeDiffFingerprint('start')); + }); +}); diff --git a/packages/github/src/pr-url.ts b/packages/github/src/pr-url.ts index f7df46e..18bf421 100644 --- a/packages/github/src/pr-url.ts +++ b/packages/github/src/pr-url.ts @@ -1,3 +1,4 @@ +import { execFileSync } from 'node:child_process'; import { exec } from './exec.js'; import type { PrBase } from './types.js'; @@ -21,7 +22,19 @@ export function parseGitHubPrUrl(url: string): { owner: string; repo: string; nu } export function checkoutPr(prNumber: number): void { - exec(`gh pr checkout ${prNumber}`); + try { + exec(`gh pr checkout ${prNumber}`); + return; + } catch (err) { + // A merged pull request usually has its branch deleted, and gh can only check out a branch. + // The forge keeps refs/pull//head either way, which is what a review actually needs. + try { + execFileSync('git', ['fetch', 'origin', `refs/pull/${prNumber}/head`], { stdio: 'pipe' }); + execFileSync('git', ['checkout', '--detach', 'FETCH_HEAD'], { stdio: 'pipe' }); + } catch { + throw err; + } + } } export function parsePrBase(json: string): PrBase { diff --git a/packages/skills/diffity-review/SKILL.md b/packages/skills/diffity-review/SKILL.md index cbb19ae..dd723d8 100644 --- a/packages/skills/diffity-review/SKILL.md +++ b/packages/skills/diffity-review/SKILL.md @@ -19,6 +19,8 @@ You are reviewing a diff and leaving inline comments using the `{{binary}} agent ## CLI Reference ``` +{{binary}} agent review-start [--note ""] +{{binary}} agent review-done {{binary}} agent standards [--json] {{binary}} agent diff {{binary}} agent list [--status open|resolved|dismissed] [--json] @@ -53,7 +55,23 @@ You are reviewing a diff and leaving inline comments using the `{{binary}} agent ## Instructions -### Step 0: Decide what to review +### Step 0: Say that you have started + +As soon as the session exists, run: + +``` +{{binary}} agent review-start --note "" +``` + +The page then shows an unmissable banner, and submitting the review to the forge is blocked +until you finish. Without it a reader cannot tell "nothing found" from "not finished looking", +and may approve the change while findings are still arriving. + +Run `{{binary}} agent review-done` as the last thing you do, **after** the comments and the +reading order are in — including when you found nothing, and including when you give up early. +Leaving a review marked in progress blocks submission indefinitely. + +### Step 0b: Decide what to review Only when no `ref` argument was given: diff --git a/packages/ui/src/components/diff/diff-page.tsx b/packages/ui/src/components/diff/diff-page.tsx index 7684d8c..eb4ef2d 100644 --- a/packages/ui/src/components/diff/diff-page.tsx +++ b/packages/ui/src/components/diff/diff-page.tsx @@ -17,6 +17,7 @@ import { DiffView, type DiffViewHandle } from './diff-view'; import { Sidebar } from '../layout/sidebar'; import { ShortcutModal } from '../layout/shortcut-modal'; import { StaleDiffBanner } from '../layout/stale-diff-banner'; +import { ReviewProgressBanner } from '../layout/review-progress-banner'; import { CheckCircleIcon } from '../icons/check-circle-icon'; import { PageLoader } from '../layout/skeleton'; import { useDiffStaleness } from '../../hooks/use-diff-staleness'; @@ -417,10 +418,14 @@ export function DiffPage() { branch={info?.branch || null} description={hideWhitespace ? `${info?.description ?? ''} · whitespace hidden` : info?.description || null} githubDetails={githubDetails} + reviewInProgress={!!info?.review?.inProgress} sessionId={sessionId} onGitHubPulled={() => queryClient.invalidateQueries({ queryKey: ['threads'] })} /> {isStale && } + {info?.review?.inProgress && ( + + )} {activeTour && ( void; onClose: () => void; } @@ -42,7 +44,7 @@ function lineLabel(thread: CommentThread): string { } export function GitHubDialog(props: GitHubDialogProps) { - const { details, threads, sessionId, onPulled, onClose } = props; + const { details, threads, sessionId, reviewInProgress, onPulled, onClose } = props; const [commentCount, setCommentCount] = useState(details.commentCount); const [submitting, setSubmitting] = useState(false); const [pulling, setPulling] = useState(false); @@ -84,7 +86,7 @@ export function GitHubDialog(props: GitHubDialogProps) { }, [onClose]); const chosen = submittable.filter(thread => selected.has(thread.id)); - const canSubmit = chosen.length > 0 || summary.trim().length > 0; + const canSubmit = !reviewInProgress && (chosen.length > 0 || summary.trim().length > 0); const toggle = (id: string) => { setSelected(prev => { @@ -263,6 +265,13 @@ export function GitHubDialog(props: GitHubDialogProps) { )} + {reviewInProgress && ( +
+ A review is still in progress — more findings may arrive. Wait for it to finish + before submitting. +
+ )} +
{(Object.keys(EVENT_LABELS) as ReviewEvent[]).map(value => { diff --git a/packages/ui/src/components/layout/review-progress-banner.tsx b/packages/ui/src/components/layout/review-progress-banner.tsx new file mode 100644 index 0000000..51a32b7 --- /dev/null +++ b/packages/ui/src/components/layout/review-progress-banner.tsx @@ -0,0 +1,38 @@ +import dayjs from 'dayjs'; +import relativeTime from 'dayjs/plugin/relativeTime'; +import type { ReviewRun } from '../../lib/api'; +import { Spinner } from '../icons/spinner'; + +dayjs.extend(relativeTime); + +interface ReviewProgressBannerProps { + review: ReviewRun; + findings: number; +} + +/** + * Deliberately loud: the difference between "nothing found" and "not finished looking" is the + * difference between approving a change and approving it too early. + */ +export function ReviewProgressBanner(props: ReviewProgressBannerProps) { + const { review, findings } = props; + + return ( +
+ + A review is still in progress + + {findings === 0 + ? 'no findings yet' + : `${findings} finding${findings === 1 ? '' : 's'} so far`} + {review.startedAt ? ` · started ${dayjs(review.startedAt).fromNow()}` : ''} + {review.note ? ` · ${review.note}` : ''} + + Wait for it to finish before approving. +
+ ); +} diff --git a/packages/ui/src/components/layout/toolbar.tsx b/packages/ui/src/components/layout/toolbar.tsx index 19f331c..ae82c12 100644 --- a/packages/ui/src/components/layout/toolbar.tsx +++ b/packages/ui/src/components/layout/toolbar.tsx @@ -133,6 +133,7 @@ export function Toolbar(props: ToolbarProps) { branch, description, githubDetails, + reviewInProgress, sessionId, onGitHubPulled, } = props; @@ -220,6 +221,7 @@ export function Toolbar(props: ToolbarProps) { {showGitHub && githubDetails && ( onGitHubPulled?.()} diff --git a/packages/ui/src/lib/api.ts b/packages/ui/src/lib/api.ts index 29501b0..979f1c3 100644 --- a/packages/ui/src/lib/api.ts +++ b/packages/ui/src/lib/api.ts @@ -47,6 +47,12 @@ export interface GitHubDetails { viewerDidAuthor: boolean; } +export interface ReviewRun { + inProgress: boolean; + startedAt: string | null; + note: string; +} + export interface RepoInfo { name: string; branch: string; @@ -56,6 +62,7 @@ export interface RepoInfo { sessionId?: string | null; github?: GitHubRemote | null; editor?: 'vscode' | null; + review?: ReviewRun | null; } export interface Commit { diff --git a/packages/ui/src/queries/info.ts b/packages/ui/src/queries/info.ts index 75b6bba..ac9d33c 100644 --- a/packages/ui/src/queries/info.ts +++ b/packages/ui/src/queries/info.ts @@ -5,5 +5,8 @@ export function repoInfoOptions(ref?: string) { return queryOptions({ queryKey: ['repo-info', ref], queryFn: () => fetchRepoInfo(ref), + // A restart on a new commit creates a new session, and comments written against the id a + // tab is still holding would land somewhere invisible. + refetchInterval: 5000, }); } diff --git a/packages/ui/tests/github-dialog-guard.test.tsx b/packages/ui/tests/github-dialog-guard.test.tsx new file mode 100644 index 0000000..a5b5241 --- /dev/null +++ b/packages/ui/tests/github-dialog-guard.test.tsx @@ -0,0 +1,70 @@ +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { render, cleanup, screen } from '@testing-library/react'; +import { GitHubDialog } from '../src/components/layout/github-dialog'; +import type { GitHubDetails } from '../src/lib/api'; +import type { CommentThread } from '../src/components/comments/types'; + +const details: GitHubDetails = { + prNumber: 671, + prTitle: 'fix: keep experiment date range in sync', + prUrl: 'https://github.com/o/r/pull/671', + prCreatedAt: '2026-08-21T10:00:00.000Z', + headSha: 'abc123', + commentCount: 0, + viewerDidAuthor: false, +}; + +function thread(id: string): CommentThread { + return { + id, + filePath: 'src/a.ts', + side: 'new', + startLine: 10, + endLine: 10, + status: 'open', + comments: [ + { id: `c-${id}`, author: { name: 'Agent', type: 'agent' }, body: 'P2: something', createdAt: '' }, + ], + }; +} + +function renderDialog(reviewInProgress: boolean) { + return render( + , + ); +} + +function submitButton(): HTMLButtonElement { + const found = screen + .getAllByRole('button') + .find(button => /as one review/i.test(button.textContent ?? '')); + if (!found) { + throw new Error('submit button not found'); + } + return found as HTMLButtonElement; +} + +afterEach(cleanup); + +describe('submitting while a review is still running', () => { + it('is refused, with the reason visible', () => { + renderDialog(true); + + expect(submitButton().disabled).toBe(true); + expect(screen.getByText(/still in progress/i)).toBeTruthy(); + }); + + it('is allowed once the review has finished', () => { + renderDialog(false); + + expect(submitButton().disabled).toBe(false); + expect(screen.queryByText(/still in progress/i)).toBeNull(); + }); +}); diff --git a/skills/diffity-review/SKILL.md b/skills/diffity-review/SKILL.md index 8605ca0..6f16cd4 100644 --- a/skills/diffity-review/SKILL.md +++ b/skills/diffity-review/SKILL.md @@ -19,6 +19,8 @@ You are reviewing a diff and leaving inline comments using the `diffity agent` C ## CLI Reference ``` +diffity agent review-start [--note ""] +diffity agent review-done diffity agent standards [--json] diffity agent diff diffity agent list [--status open|resolved|dismissed] [--json] @@ -53,7 +55,23 @@ diffity agent tour-done --tour ## Instructions -### Step 0: Decide what to review +### Step 0: Say that you have started + +As soon as the session exists, run: + +``` +diffity agent review-start --note "" +``` + +The page then shows an unmissable banner, and submitting the review to the forge is blocked +until you finish. Without it a reader cannot tell "nothing found" from "not finished looking", +and may approve the change while findings are still arriving. + +Run `diffity agent review-done` as the last thing you do, **after** the comments and the +reading order are in — including when you found nothing, and including when you give up early. +Leaving a review marked in progress blocks submission indefinitely. + +### Step 0b: Decide what to review Only when no `ref` argument was given: