diff --git a/packages/cli/src/agent.ts b/packages/cli/src/agent.ts index 91f17e8..7b0144e 100644 --- a/packages/cli/src/agent.ts +++ b/packages/cli/src/agent.ts @@ -16,6 +16,7 @@ import { } from './threads.js'; import { answerLiveRequest, type LiveRequest } from './live.js'; import { clampClientWait } from './live-wait.js'; +import { directiveFor } from './live-intent.js'; import { findInstanceForRepo, type RegistryEntry } from './registry.js'; import { createHash } from 'node:crypto'; import { createTour, addTourStep, updateTourStatus, deleteTour, deleteToursForSession } from './tours.js'; @@ -313,9 +314,15 @@ Examples: const startedAt = Date.now(); let payload: { request: LiveRequest | null }; try { - const res = await fetch(`http://127.0.0.1:${instance.port}/api/live/claim?wait=${wait}`, { - method: 'POST', - }); + // Park on the session the server is serving, not on whatever the shared current-session + // file last named — those differ whenever another worktree has opened a review since. + const info = await fetch(`http://127.0.0.1:${instance.port}/api/info`); + const sessionId = info.ok + ? ((await info.json()) as { sessionId?: string }).sessionId + : undefined; + const claimUrl = `http://127.0.0.1:${instance.port}/api/live/claim?wait=${wait}` + + (sessionId ? `&session=${encodeURIComponent(sessionId)}` : ''); + const res = await fetch(claimUrl, { method: 'POST' }); if (!res.ok) { console.error(pc.red(`Could not wait for a request: ${res.status} ${await res.text()}`)); process.exitCode = 1; @@ -347,15 +354,10 @@ Examples: // stdout is the request, so a script can parse it. The directive goes to stderr, because the // turn this wakes up may be a long way from whatever armed the loop. - const mayChange = payload.request.mayChangeCode !== false; console.error( pc.cyan( - 'A request came back from the review page. ' - + (mayChange - ? 'Answer it in the thread, amend the finding it is about, or make the change' - : 'Answer it in the thread or amend the finding it is about — this pull request is ' - + 'somebody else\'s, so do not edit its code') - + ' — then re-arm with `agent await`. The diffity-live skill has the detail.', + `${directiveFor(payload.request.intent, payload.request.mayChangeCode !== false)}\n` + + 'Then re-arm with `agent await`. The diffity-live skill has the detail.', ), ); console.log(JSON.stringify(payload.request, null, 2)); diff --git a/packages/cli/src/db.ts b/packages/cli/src/db.ts index 5b23d76..1feccbf 100644 --- a/packages/cli/src/db.ts +++ b/packages/cli/src/db.ts @@ -141,6 +141,7 @@ function migrateDb(db: DatabaseSync): void { // because "asked but not yet picked up" and "picked up but not answered" are both waiting, and // the page says different things about them. addColumn(db, 'comments', 'live_requested_at', 'TEXT'); + addColumn(db, 'comments', 'live_intent', 'TEXT'); addColumn(db, 'comments', 'live_claimed_at', 'TEXT'); addColumn(db, 'comments', 'live_answered_at', 'TEXT'); addColumn(db, 'comment_threads', 'submitted_at', 'TEXT'); diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 36434ea..cc14bd4 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -72,6 +72,8 @@ program .option('--dark', 'Open in dark mode (default: light)') .option('--unified', 'Open in unified view (default: split)') .option('--new', 'Stop existing instance and start fresh') + .option('--work', 'You are working on this branch, so the agent may change code') + .option('--review', 'You are reviewing it, so the agent may not — even if you wrote it') .addHelpText('after', ` Common usage: $ diffity See all uncommitted changes @@ -331,6 +333,7 @@ range syntax (main..feature, main...feature) also work.`) prNumber: parsedPrNumber, version: pkg.version, registryInfo: { repoRoot, repoHash, repoName }, + purpose: opts.work ? 'work' : opts.review ? 'review' : undefined, }); const urlParams = new URLSearchParams({ ref: effectiveRef }); if (opts.dark) { diff --git a/packages/cli/src/live-intent.ts b/packages/cli/src/live-intent.ts new file mode 100644 index 0000000..f3a84a8 --- /dev/null +++ b/packages/cli/src/live-intent.ts @@ -0,0 +1,23 @@ +/** What the reader pressed: a question, or a request for a change. */ +export type LiveIntent = 'ask' | 'act'; + +/** Anything not plainly `act` is a question: least privilege, and it covers old and malformed alike. */ +export function normaliseIntent(value: unknown): LiveIntent { + return value === 'act' ? 'act' : 'ask'; +} + +/** Said in plain language, because that is what the agent acts on. */ +export function directiveFor(intent: LiveIntent, mayChangeCode: boolean): string { + if (intent === 'ask') { + return 'The reader asked a question. Answer it in the thread, or amend the finding it is about. ' + + 'Do not change code — they pressed Ask, not Act.'; + } + + if (!mayChangeCode) { + return 'The reader asked for a change, but this pull request is somebody else\'s. ' + + 'Do not change code. Answer in the thread, or amend the finding, and say that is why.'; + } + + return 'The reader asked for a change. Read it, make the change, and reply in the thread with what ' + + 'you did. Do not commit, push or merge.'; +} diff --git a/packages/cli/src/live-permissions.ts b/packages/cli/src/live-permissions.ts index bdb3e6e..f7278d7 100644 --- a/packages/cli/src/live-permissions.ts +++ b/packages/cli/src/live-permissions.ts @@ -12,3 +12,28 @@ export function mayChangeCode(pullRequest: { viewerDidAuthor?: boolean } | null) } return pullRequest.viewerDidAuthor === true; } + +export type SessionPurpose = 'work' | 'review'; + +export function normalisePurpose(value: unknown): SessionPurpose | undefined { + return value === 'work' || value === 'review' ? value : undefined; +} + +/** + * Authorship is a proxy for whose work this is, and it stops being one the moment work is handed + * over: take over a colleague's branch and the pull request still says they opened it. + * + * The agent launching diffity knows which it is doing, so it can say — once, at launch, for that + * server only. Not a setting: there is nothing to leave switched on, because it dies with the + * process. Unsaid still means derived from authorship. + */ +export function resolveMayChangeCode( + purpose: SessionPurpose | undefined, + pullRequest: { viewerDidAuthor?: boolean } | null, +): boolean { + const said = normalisePurpose(purpose); + if (said) { + return said === 'work'; + } + return mayChangeCode(pullRequest); +} diff --git a/packages/cli/src/live.ts b/packages/cli/src/live.ts index cb4c85d..43c4759 100644 --- a/packages/cli/src/live.ts +++ b/packages/cli/src/live.ts @@ -1,4 +1,5 @@ import { getDb, queryOne } from './db.js'; +import { normaliseIntent, type LiveIntent } from './live-intent.js'; /** * A question or an instruction the reader left for the agent, taken from the comment it was written @@ -25,6 +26,8 @@ export interface LiveRequest { * filled in by the route that hands the request over. */ mayChangeCode?: boolean; + /** What the reader pressed. A question must not turn into an edit. */ + intent: LiveIntent; } export interface LiveRequestStamp { @@ -34,10 +37,10 @@ export interface LiveRequestStamp { requestedAt: string | null; } -export function requestLive(commentId: string): LiveRequestStamp { +export function requestLive(commentId: string, intent: LiveIntent = 'ask'): LiveRequestStamp { getDb() - .prepare("UPDATE comments SET live_requested_at = datetime('now') WHERE id = ?") - .run(commentId); + .prepare("UPDATE comments SET live_requested_at = datetime('now'), live_intent = ? WHERE id = ?") + .run(intent, commentId); const row = queryOne<{ session_id: string; live_requested_at: string | null }>( `SELECT t.session_id, c.live_requested_at FROM comments c JOIN comment_threads t ON t.id = c.thread_id @@ -73,19 +76,20 @@ export function claimNextLiveRequest(sessionId: string): LiveRequest | null { return null; } - return ( - queryOne( - `SELECT c.id AS commentId, c.thread_id AS threadId, c.body AS body, - c.author_name AS authorName, t.file_path AS filePath, t.side AS side, + const request = queryOne( + `SELECT c.id AS commentId, c.thread_id AS threadId, c.body AS body, + c.author_name AS authorName, c.live_intent AS intent, + t.file_path AS filePath, t.side AS side, t.start_line AS startLine, t.end_line AS endLine, (SELECT f.body FROM comments f WHERE f.thread_id = t.id AND COALESCE(f.kind, 'review') = 'review' ORDER BY f.created_at ASC, f.rowid ASC LIMIT 1) AS findingBody FROM comments c JOIN comment_threads t ON t.id = c.thread_id WHERE c.id = ?`, - claimed.id, - ) ?? null + claimed.id, ); + + return request ? { ...request, intent: normaliseIntent(request.intent) } : null; } /** @@ -99,6 +103,21 @@ export function answerLiveRequest(commentIdOrPrefix: string): boolean { return Number(result.changes ?? 0) > 0; } +/** + * Requests an agent has taken and not yet answered. Between the two it is not parked on the claim + * route, so presence alone would report nobody there while somebody is working on your question. + */ +export function liveWorkingCount(sessionId: string): number { + const row = queryOne<{ n: number }>( + `SELECT COUNT(*) AS n FROM comments c JOIN comment_threads t ON t.id = c.thread_id + WHERE t.session_id = ? + AND c.live_claimed_at IS NOT NULL + AND c.live_answered_at IS NULL`, + sessionId, + ); + return row?.n ?? 0; +} + /** How many requests are waiting for somebody to pick them up. */ export function pendingLiveCount(sessionId: string): number { const row = queryOne<{ n: number }>( diff --git a/packages/cli/src/review-routes.ts b/packages/cli/src/review-routes.ts index 1f7e28c..236313b 100644 --- a/packages/cli/src/review-routes.ts +++ b/packages/cli/src/review-routes.ts @@ -13,6 +13,7 @@ import { type CommentKind, } from './threads.js'; import { requestLive, notifyLiveListeners } from './live.js'; +import { normaliseIntent } from './live-intent.js'; import { getCurrentSession, resolveSessionId } from './session.js'; import { sendJson, sendError, withJsonBody } from './http-utils.js'; @@ -50,7 +51,7 @@ export function handleReviewRoute(req: IncomingMessage, res: ServerResponse, pat if (pathname === '/api/threads' && req.method === 'POST') { withJsonBody(res, req, 'Failed to create thread', (body) => { - const { sessionId: sid, filePath, side, startLine, endLine, body: commentBody, author, anchorContent, kind, live } = body; + const { sessionId: sid, filePath, side, startLine, endLine, body: commentBody, author, anchorContent, kind, live, intent } = body; if (!sid || !filePath || !side || typeof startLine !== 'number' || typeof endLine !== 'number' || !commentBody || !author) { sendError(res, 400, 'Missing required fields'); return; @@ -63,7 +64,7 @@ export function handleReviewRoute(req: IncomingMessage, res: ServerResponse, pat threadKind, ); if (live === true && threadKind === 'aside') { - const stamp = requestLive(thread.comments[0].id); + const stamp = requestLive(thread.comments[0].id, normaliseIntent(intent)); thread.comments[0].liveRequestedAt = stamp.requestedAt; notifyLiveListeners(stamp.sessionId); } @@ -75,7 +76,7 @@ export function handleReviewRoute(req: IncomingMessage, res: ServerResponse, pat const threadReplyMatch = pathname.match(/^\/api\/threads\/([^/]+)\/reply$/); if (threadReplyMatch && req.method === 'POST') { withJsonBody(res, req, 'Failed to add reply', (body) => { - const { body: commentBody, author, kind, live } = body; + const { body: commentBody, author, kind, live, intent } = body; if (!commentBody || !author) { sendError(res, 400, 'Missing body or author'); return; @@ -91,7 +92,7 @@ export function handleReviewRoute(req: IncomingMessage, res: ServerResponse, pat // request's author, and it is going to the forge rather than to a listener here. let requestedAt: string | null = null; if (live === true && commentKind === 'aside') { - const stamp = requestLive(comment.id); + const stamp = requestLive(comment.id, normaliseIntent(intent)); requestedAt = stamp.requestedAt; notifyLiveListeners(stamp.sessionId); } diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index f441376..89e88b2 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -47,9 +47,10 @@ import { type ReviewEvent, } from '@diffity/github'; import { findOrCreateSession, resolveSessionId } from './session.js'; -import { mayChangeCode } from './live-permissions.js'; +import { resolveMayChangeCode, type SessionPurpose } from './live-permissions.js'; import { liveListenerCount, + liveWorkingCount, pendingLiveCount, reclaimStaleLiveRequests, waitForLiveRequest, @@ -190,6 +191,11 @@ function isSameOriginRequest(req: IncomingMessage): boolean { } interface ServerOptions { + /** + * What the agent launching this said it was here for. Unsaid means derived from who wrote the + * pull request, which is wrong exactly when work has been handed over. + */ + purpose?: SessionPurpose; port: number; portIsExplicit?: boolean; diffArgs: string[]; @@ -247,6 +253,7 @@ interface ServerResult { export function startServer(options: ServerOptions): Promise { const { + purpose, port, portIsExplicit, diffArgs, @@ -365,14 +372,25 @@ export function startServer(options: ServerOptions): Promise { // as one arms and answers, and react-query keeps an unchanged object's identity — so // carrying this on the info payload made every consumer of it re-render each time a // listener came or went, which reads as the page reloading under you. + // The page asks about the session it is showing, which is the one for its ref — not + // whichever session the ambient current-session file last named, which is shared by every + // worktree using this data directory. + const liveSessionId = (): string => { + const asked = url.searchParams.get('session'); + if (asked) { + return resolveSessionId(asked); + } + return findOrCreateSession(url.searchParams.get('ref') || effectiveRef).id; + }; + if (pathname === '/api/live/status') { - const sid = resolveSessionId(url.searchParams.get('session')); + const sid = liveSessionId(); sendJson(res, { - // A comment box that drives an agent is only as safe as the loopback bind, so live - // mode is not offered at all when the server is reachable from elsewhere. enabled: isLoopbackBind(getBindHost()), listening: sid ? liveListenerCount(sid) > 0 : false, + working: sid ? liveWorkingCount(sid) > 0 : false, waiting: sid ? pendingLiveCount(sid) : 0, + mayChangeCode: resolveMayChangeCode(purpose, authorship()), }); return; } @@ -382,7 +400,7 @@ export function startServer(options: ServerOptions): Promise { sendError(res, 403, 'Live mode is only available on a loopback bind'); return; } - const sid = resolveSessionId(url.searchParams.get('session')); + const sid = liveSessionId(); if (!sid) { sendError(res, 400, 'No review session'); return; @@ -411,7 +429,7 @@ export function startServer(options: ServerOptions): Promise { } // Carried on the request rather than left for the agent to look up: a rule nobody // has to remember is a rule that holds. - sendJson(res, { request: { ...request, mayChangeCode: mayChangeCode(authorship()) } }); + sendJson(res, { request: { ...request, mayChangeCode: resolveMayChangeCode(purpose, authorship()) } }); }, err => { if (!res.writableEnded && !listenerGone.signal.aborted) { diff --git a/packages/cli/src/threads.ts b/packages/cli/src/threads.ts index d120986..d98a07e 100644 --- a/packages/cli/src/threads.ts +++ b/packages/cli/src/threads.ts @@ -16,6 +16,7 @@ export interface ThreadComment { kind: CommentKind; createdAt: string; liveRequestedAt: string | null; + liveIntent: string | null; liveClaimedAt: string | null; liveAnsweredAt: string | null; } @@ -65,6 +66,7 @@ interface CommentRow { kind?: string | null; created_at: string; live_requested_at?: string | null; + live_intent?: string | null; live_claimed_at?: string | null; live_answered_at?: string | null; } @@ -96,6 +98,7 @@ function rowToComment(row: CommentRow): ThreadComment { kind: (row.kind as CommentKind | null) ?? 'review', createdAt: row.created_at, liveRequestedAt: row.live_requested_at ?? null, + liveIntent: row.live_intent ?? null, liveClaimedAt: row.live_claimed_at ?? null, liveAnsweredAt: row.live_answered_at ?? null, }; @@ -202,6 +205,7 @@ export function createThread( kind, createdAt: now, liveRequestedAt: null, + liveIntent: null, liveClaimedAt: null, liveAnsweredAt: null, }], @@ -216,6 +220,7 @@ interface JoinedRow extends ThreadRow { c_kind: string | null; c_created_at: string | null; c_live_requested_at: string | null; + c_live_intent: string | null; c_live_claimed_at: string | null; c_live_answered_at: string | null; } @@ -230,7 +235,8 @@ export function getThreadsForSession(sessionId: string, status?: ThreadStatus): SELECT t.*, c.id AS c_id, c.author_name AS c_author_name, c.author_type AS c_author_type, c.body AS c_body, c.kind AS c_kind, c.created_at AS c_created_at, - c.live_requested_at AS c_live_requested_at, c.live_claimed_at AS c_live_claimed_at, + c.live_requested_at AS c_live_requested_at, c.live_intent AS c_live_intent, + c.live_claimed_at AS c_live_claimed_at, c.live_answered_at AS c_live_answered_at FROM comment_threads t LEFT JOIN comments c ON c.thread_id = t.id @@ -253,6 +259,7 @@ export function getThreadsForSession(sessionId: string, status?: ThreadStatus): kind: (row.c_kind as CommentKind | null) ?? 'review', createdAt: row.c_created_at!, liveRequestedAt: row.c_live_requested_at ?? null, + liveIntent: row.c_live_intent ?? null, liveClaimedAt: row.c_live_claimed_at ?? null, liveAnsweredAt: row.c_live_answered_at ?? null, }); @@ -307,6 +314,7 @@ export function addReply( kind, createdAt: now, liveRequestedAt: null, + liveIntent: null, liveClaimedAt: null, liveAnsweredAt: null, }; diff --git a/packages/cli/tests/change-permission.test.ts b/packages/cli/tests/change-permission.test.ts new file mode 100644 index 0000000..701ad87 --- /dev/null +++ b/packages/cli/tests/change-permission.test.ts @@ -0,0 +1,31 @@ +import { describe, it, expect } from 'vitest'; +import { resolveMayChangeCode } from '../src/live-permissions.js'; + +describe('whether the agent may change code here', () => { + const mine = { viewerDidAuthor: true }; + const theirs = { viewerDidAuthor: false }; + + // Nothing said: fall back to who wrote it, which is what it did before there was a flag. + it('derives it from authorship when nobody said', () => { + expect(resolveMayChangeCode(undefined, mine)).toBe(true); + expect(resolveMayChangeCode(undefined, theirs)).toBe(false); + expect(resolveMayChangeCode(undefined, null)).toBe(true); + }); + + // Taking over somebody else's branch is the case authorship gets wrong, and the agent that + // launched diffity is the one that knows. + it('lets the launcher say this is work rather than review', () => { + expect(resolveMayChangeCode('work', theirs)).toBe(true); + }); + + // And the other way: reviewing your own pull request should not invite edits mid-review. + it('lets the launcher say this is review rather than work', () => { + expect(resolveMayChangeCode('review', mine)).toBe(false); + expect(resolveMayChangeCode('review', null)).toBe(false); + }); + + it('ignores anything it does not recognise rather than guessing', () => { + expect(resolveMayChangeCode('sideways' as never, theirs)).toBe(false); + expect(resolveMayChangeCode('sideways' as never, mine)).toBe(true); + }); +}); diff --git a/packages/cli/tests/live-intent.test.ts b/packages/cli/tests/live-intent.test.ts new file mode 100644 index 0000000..12700d7 --- /dev/null +++ b/packages/cli/tests/live-intent.test.ts @@ -0,0 +1,44 @@ +import { describe, it, expect } from 'vitest'; +import { normaliseIntent, directiveFor } from '../src/live-intent.js'; + +describe('what the reader asked for', () => { + it('takes an explicit intent', () => { + expect(normaliseIntent('act')).toBe('act'); + expect(normaliseIntent('ask')).toBe('ask'); + }); + + // Least privilege: a request that does not say what it wants gets an answer, not an edit. That + // covers rows written before intent existed as well as anything malformed. + it('answers rather than acts when it does not say', () => { + expect(normaliseIntent(undefined)).toBe('ask'); + expect(normaliseIntent(null)).toBe('ask'); + expect(normaliseIntent('anything else')).toBe('ask'); + }); +}); + +describe('what the agent is told to do with it', () => { + it('forbids changing code for a question, in as many words', () => { + const directive = directiveFor('ask', true); + + expect(directive).toContain('Do not change code'); + }); + + it('allows a change when one was asked for and is permitted', () => { + const directive = directiveFor('act', true); + + expect(directive).toContain('make the change'); + expect(directive).not.toContain('Do not change code'); + }); + + // Asking for a change on somebody else's pull request is still not permission to make one. + it('refuses a change on a pull request the reader did not write', () => { + const directive = directiveFor('act', false); + + expect(directive).toContain('Do not change code'); + expect(directive).toContain('somebody else'); + }); + + it('says the same about a question either way', () => { + expect(directiveFor('ask', false)).toContain('Do not change code'); + }); +}); diff --git a/packages/cli/tests/live-requests.test.ts b/packages/cli/tests/live-requests.test.ts index ce3f9bc..650905a 100644 --- a/packages/cli/tests/live-requests.test.ts +++ b/packages/cli/tests/live-requests.test.ts @@ -44,11 +44,14 @@ async function finding(body = 'P2: a finding') { async function drainRequests() { - const { claimNextLiveRequest } = await import('../src/live.js'); + const { claimNextLiveRequest, answerLiveRequest } = await import('../src/live.js'); const s = await session(); - while (claimNextLiveRequest(s.id)) { - // Sessions on one branch share their open threads by design, so earlier cases leave requests - // in this queue. A case about "the next one" has to start from empty. + // Answered, not merely claimed: sessions on one branch share their open threads, and a claim + // without an answer leaves the session looking like an agent is still working on it. + let taken = claimNextLiveRequest(s.id); + while (taken) { + answerLiveRequest(taken.commentId); + taken = claimNextLiveRequest(s.id); } } @@ -345,3 +348,40 @@ describe('a listener whose connection goes away', () => { expect(liveListenerCount(s.id)).toBe(0); }); }); + +describe('while an agent is busy with a request', () => { + // Between taking a request and answering it the listener is not parked, so "listening" is false + // — and the page said "No agent" at exactly the moment one was working on your question. + it('is reported as working, not as absent', async () => { + const { addReply } = await import('../src/threads.js'); + const { requestLive, claimNextLiveRequest, liveWorkingCount } = await import('../src/live.js'); + await drainRequests(); + const s = await session(); + const thread = await finding(); + const asked = addReply(thread.id, 'busy with this', you, 'aside'); + requestLive(asked.id); + + // A delta, not an absolute: other cases in this file claim without answering, and every + // session on this branch shares its threads. + const before = liveWorkingCount(s.id); + claimNextLiveRequest(s.id); + + expect(liveWorkingCount(s.id)).toBe(before + 1); + }); + + it('stops being busy once it has answered', async () => { + const { addReply } = await import('../src/threads.js'); + const { requestLive, claimNextLiveRequest, answerLiveRequest, liveWorkingCount } = await import('../src/live.js'); + await drainRequests(); + const s = await session(); + const thread = await finding(); + const asked = addReply(thread.id, 'answer me', you, 'aside'); + requestLive(asked.id); + claimNextLiveRequest(s.id); + const busy = liveWorkingCount(s.id); + + answerLiveRequest(asked.id); + + expect(liveWorkingCount(s.id)).toBe(busy - 1); + }); +}); diff --git a/packages/cli/tests/purpose-flag.test.ts b/packages/cli/tests/purpose-flag.test.ts new file mode 100644 index 0000000..4d0ccec --- /dev/null +++ b/packages/cli/tests/purpose-flag.test.ts @@ -0,0 +1,60 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, rmSync, mkdirSync } 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-purpose-')); + repoDir = join(root, 'repo'); + mkdirSync(repoDir); + 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.ts'), 'const a = 1;\n'); + execFileSync('git', ['add', '.'], { cwd: repoDir, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'init'], { cwd: repoDir, stdio: 'pipe' }); + writeFileSync(join(repoDir, 'a.ts'), 'const a = 2;\n'); + 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 }); +}); + +async function statusWith(purpose?: 'work' | 'review') { + const { startServer } = await import('../src/server.js'); + const started = await startServer({ port: 0, diffArgs: [], effectiveRef: 'work', purpose }); + try { + const res = await fetch(`http://127.0.0.1:${started.port}/api/live/status`); + return (await res.json()) as { mayChangeCode: boolean }; + } finally { + started.close(); + } +} + +describe('what the launcher said it was here for', () => { + // The flag is parsed in index.ts, carried through ServerOptions and read in two places. All of + // that was verified by hand and by nothing else, on the path that decides whether an agent may + // edit somebody else's branch. + it('reaches the page as permission to change code', async () => { + expect((await statusWith('work')).mayChangeCode).toBe(true); + }); + + it('reaches the page as a refusal', async () => { + expect((await statusWith('review')).mayChangeCode).toBe(false); + }); + + // No pull request here, so authorship says it is the reader's own working tree. + it('falls back to authorship when nothing was said', async () => { + expect((await statusWith()).mayChangeCode).toBe(true); + }); +}); diff --git a/packages/skills/diffity-live/SKILL.md b/packages/skills/diffity-live/SKILL.md index 02349a7..f884b71 100644 --- a/packages/skills/diffity-live/SKILL.md +++ b/packages/skills/diffity-live/SKILL.md @@ -31,9 +31,35 @@ never did is guessing. `findingBody` is usually the thing being asked about. Read it before the question. -## Choose one of three +## Re-arm first -Read `body` and decide. Do not do more than was asked. +Before answering, put the loop back: + +``` +{{binary}} agent await --timeout 240 +``` + +Background command, as always. The reader is looking at the page having just asked, and between +taking their request and answering it there is nobody parked — so the page said "No agent" at exactly +the moment they were watching. Re-arming first closes that window, and a second request arriving +while you write is queued rather than lost. + +Nothing about answering changes; it just happens second. + +## The reader already chose + +The request carries an `intent`, because there are two buttons in the page and they mean different +things. `await` prints what it means in plain words before the payload; that line is the instruction, +not a summary of it. + +- **`ask`** — a question. Answer it, or amend the finding it is about. **Do not change code**, however + obvious the change looks. They pressed Ask. +- **`act`** — a request for a change. Make it, and say what you did. + +An `intent` that is absent or unrecognised is a question. If `mayChangeCode` is false the answer is +the same whatever they pressed: this pull request is somebody else's. + +Read `body` and decide *how* to do what was asked. Do not do more than was asked. **Answer it.** A question about the finding, the code, or your reasoning. @@ -56,7 +82,8 @@ does. Amend the **finding**, not the aside. If the finding has already been sent, `amend` tells you so — pass that on rather than letting the reader think the pull request has changed. -**Make the change.** Only on your own work. Read the rule below before you edit anything. +**Make the change.** Only when the intent is `act` and `mayChangeCode` is not false. Read the rule +below before you edit anything. ``` # make the edit, then: @@ -68,19 +95,21 @@ merge — those wait to be asked for, here as everywhere. ### When you must not change code -Run `{{binary}} agent live-status` if you are unsure. Editing is off the table when the diff is +Run `{{binary}} agent live-status` if you are unsure. + +Whether changes are allowed is decided when diffity is launched. `--review` says you are reviewing +somebody's change, `--work` says you are working on the branch — which is the case authorship gets +wrong, since taking over a colleague's branch leaves their name on the pull request. Said neither +way, it falls back to who wrote it. It lasts as long as that server and no longer, so a reader who +hands work over restarts rather than unticking something. Editing is off the table when the diff is somebody else's pull request, however the conversation goes: reviewing is not editing, and a reader asking a follow-up has not asked you to rewrite their branch. Answer and amend instead, and say that is what you did. -## Then go back to waiting - -``` -{{binary}} agent await --timeout 240 -``` +## When the wait ends on its own -Background command, same as before. Exit 3 means nothing was asked — re-arm without saying anything. -Anything else means the server is gone; say so once and stop, rather than looping on a dead port. +Exit 3 means nothing was asked — re-arm without saying anything. Anything else means the server is +gone; say so once and stop, rather than looping on a dead port. ## Keep it short diff --git a/packages/skills/diffity-review/SKILL.md b/packages/skills/diffity-review/SKILL.md index e10fe02..bb0ffed 100644 --- a/packages/skills/diffity-review/SKILL.md +++ b/packages/skills/diffity-review/SKILL.md @@ -100,7 +100,10 @@ The review needs a running session whose ref matches the requested ref. A ref mi - If refs **don't match** → restart: run `{{binary}} --no-open --new` (or `{{binary}} --no-open --new` if no ref). The `--new` flag kills the old session and starts a fresh one. Use Bash tool with `run_in_background: true`. Wait 2 seconds, then verify with `{{binary}} list --json` and note the port. - If **no ref was requested** and the running session's ref is not `"work"` → restart with `{{binary}} --no-open --new` (the running session is for a named ref, but we need working-tree). 3. If **no session is running** for this repo, start one in the background: - - Command: `{{binary}} --no-open` (or `{{binary}} --no-open` if no ref) + - Command: `{{binary}} --no-open --review` (or `{{binary}} --no-open --review` if no ref) + - `--review` says what you are here for. Reviewing is not editing, so it keeps Act off the + comment box and refuses a change request even on a pull request the reader wrote themselves. + Authorship alone cannot tell the difference; you can. - Use Bash tool with `run_in_background: true` - Wait 2 seconds, then verify with `{{binary}} list --json` and note the port. diff --git a/packages/ui/src/components/comments/comment-bubble.tsx b/packages/ui/src/components/comments/comment-bubble.tsx index 1d40a1a..73e860c 100644 --- a/packages/ui/src/components/comments/comment-bubble.tsx +++ b/packages/ui/src/components/comments/comment-bubble.tsx @@ -3,7 +3,8 @@ import type { Comment } from './types'; import { PencilIcon } from '../icons/pencil-icon'; import { TrashIcon } from '../icons/trash-icon'; import { MarkdownContent } from '../layout/markdown-content'; -import { isAside, requestStateOf, type RequestState } from '../../lib/live-mode'; +import { isAside, intentOf, requestStateOf, type RequestState } from '../../lib/live-mode'; +import { clampEditHeight, MIN_EDIT_ROWS } from '../../lib/edit-box-size'; interface CommentBubbleProps { comment: Comment; @@ -13,10 +14,10 @@ interface CommentBubbleProps { // A request can sit unanswered for half a minute, and without this that looks like nothing // happening at all. -const REQUEST_LABELS: Record = { - waiting: 'asked', - working: 'agent is on it', - answered: 'answered', +const REQUEST_LABELS: Record<'ask' | 'act', Record> = { + ask: { waiting: 'asked', working: 'agent is on it', answered: 'answered' }, + // Now that the two mean different things, "asked" would not say which happened. + act: { waiting: 'change asked for', working: 'agent is on it', answered: 'done' }, }; const REQUEST_TITLES: Record = { @@ -87,6 +88,18 @@ export function CommentBubble(props: CommentBubbleProps) { } }, [isEditing]); + // The element knows its own wrapped height; a character count only guesses at it, and guesses + // differently in split and unified view. Clamped, because past a point the box would push the + // diff off screen and scrolling inside it is the lesser evil. + useEffect(() => { + const box = textareaRef.current; + if (!isEditing || !box) { + return; + } + box.style.height = 'auto'; + box.style.height = `${clampEditHeight(box.scrollHeight)}px`; + }, [isEditing, editBody]); + const handleSave = () => { const trimmed = editBody.trim(); if (!trimmed || trimmed === comment.body) { @@ -140,7 +153,7 @@ export function CommentBubble(props: CommentBubbleProps) { className={`text-[10px] px-1.5 py-0.5 rounded-full font-medium ${REQUEST_STYLES[requestState]}`} title={REQUEST_TITLES[requestState]} > - {REQUEST_LABELS[requestState]} + {REQUEST_LABELS[intentOf(comment)][requestState]} )} {!isEditing && ( @@ -170,8 +183,8 @@ export function CommentBubble(props: CommentBubbleProps) { value={editBody} onChange={(e) => setEditBody(e.target.value)} onKeyDown={handleKeyDown} - rows={3} - className="w-full px-3 py-2 text-sm bg-bg-tertiary text-text resize-y outline-none rounded-md min-h-[60px]" + rows={MIN_EDIT_ROWS} + className="w-full px-3 py-2 text-sm bg-bg-tertiary text-text resize-y outline-none rounded-md overflow-y-auto" />
diff --git a/packages/ui/src/components/comments/comment-form-row.tsx b/packages/ui/src/components/comments/comment-form-row.tsx index d200bb1..a4366dd 100644 --- a/packages/ui/src/components/comments/comment-form-row.tsx +++ b/packages/ui/src/components/comments/comment-form-row.tsx @@ -13,11 +13,12 @@ interface CommentFormRowProps { viewMode?: 'unified' | 'split'; /** Hands a brand-new comment to the agent — asking about code nobody has commented on yet. */ onAsk?: (filePath: string, side: CommentSide, startLine: number, endLine: number, body: string, author: CommentAuthor) => void; + onAct?: (filePath: string, side: CommentSide, startLine: number, endLine: number, body: string, author: CommentAuthor) => void; askIsHeard?: boolean; } export function CommentFormRow(props: CommentFormRowProps) { - const { colSpan, filePath, side, startLine, endLine, currentAuthor, onSubmit, onCancel, viewMode, onAsk, askIsHeard } = props; + const { colSpan, filePath, side, startLine, endLine, currentAuthor, onSubmit, onCancel, viewMode, onAsk, onAct, askIsHeard } = props; const lineLabel = startLine === endLine ? `${startLine}` @@ -28,6 +29,7 @@ export function CommentFormRow(props: CommentFormRowProps) { onSubmit(filePath, side, startLine, endLine, body, currentAuthor)} onAsk={onAsk && ((body) => onAsk(filePath, side, startLine, endLine, body, currentAuthor))} + onAct={onAct && ((body) => onAct(filePath, side, startLine, endLine, body, currentAuthor))} askIsHeard={askIsHeard} onCancel={onCancel} lineLabel={`Add a comment on line${startLine !== endLine ? 's' : ''} ${lineLabel}`} diff --git a/packages/ui/src/components/comments/comment-form.tsx b/packages/ui/src/components/comments/comment-form.tsx index aa8edd1..6a274a5 100644 --- a/packages/ui/src/components/comments/comment-form.tsx +++ b/packages/ui/src/components/comments/comment-form.tsx @@ -8,18 +8,23 @@ interface CommentFormProps { autoFocus?: boolean; lineLabel?: string; /** - * Hands the text to the agent instead of leaving it for whoever wrote the code. Absent when no - * agent can be reached, so the button is never offered where it would do nothing. + * Hands the text to the agent as a question. Absent when no agent can be reached, so the button + * is never offered where it would do nothing. */ onAsk?: (body: string) => void; - /** Whether an agent is waiting right now, which changes what the button promises. */ + /** + * Asks the agent to make the change. Absent when the diff is somebody else's pull request, so it + * is not offered and then refused. + */ + onAct?: (body: string) => void; + /** Whether an agent is waiting right now, which changes what the buttons promise. */ askIsHeard?: boolean; } export function CommentForm(props: CommentFormProps) { const { onSubmit, onCancel, placeholder = 'Leave a comment', submitLabel = 'Comment', - autoFocus = true, lineLabel, onAsk, askIsHeard, + autoFocus = true, lineLabel, onAsk, onAct, askIsHeard, } = props; const [body, setBody] = useState(''); const textareaRef = useRef(null); @@ -39,12 +44,12 @@ export function CommentForm(props: CommentFormProps) { setBody(''); }; - const handleAsk = () => { + const handOver = (to?: (body: string) => void) => () => { const trimmed = body.trim(); - if (!trimmed || !onAsk) { + if (!trimmed || !to) { return; } - onAsk(trimmed); + to(trimmed); setBody(''); }; @@ -88,16 +93,30 @@ export function CommentForm(props: CommentFormProps) { {onAsk && ( + )} + {onAct && ( + )}
diff --git a/packages/ui/src/components/diff/diff-view.tsx b/packages/ui/src/components/diff/diff-view.tsx index 4185822..fff7784 100644 --- a/packages/ui/src/components/diff/diff-view.tsx +++ b/packages/ui/src/components/diff/diff-view.tsx @@ -63,6 +63,8 @@ interface DiffViewProps { onRefreshFile?: (path: string) => void; onAskThread?: (filePath: string, side: CommentSide, startLine: number, endLine: number, body: string, author: CommentAuthor) => void; onAskReply?: (threadId: string, body: string, author: CommentAuthor) => void; + onActThread?: (filePath: string, side: CommentSide, startLine: number, endLine: number, body: string, author: CommentAuthor) => void; + onActReply?: (threadId: string, body: string, author: CommentAuthor) => void; askIsHeard?: boolean; } @@ -98,6 +100,8 @@ export function DiffView(props: DiffViewProps) { onRefreshFile, onAskThread, onAskReply, + onActThread, + onActReply, askIsHeard, } = props; const { highlight } = useHighlighter(); @@ -347,6 +351,8 @@ export function DiffView(props: DiffViewProps) { onRefreshFile={onRefreshFile} onAskThread={onAskThread} onAskReply={onAskReply} + onActThread={onActThread} + onActReply={onActReply} askIsHeard={askIsHeard} tourMarks={tourMarksByFile?.get(filePath)} activeStepIndex={activeStepIndex} diff --git a/packages/ui/src/components/diff/file-block.tsx b/packages/ui/src/components/diff/file-block.tsx index 38e5f3d..a55101d 100644 --- a/packages/ui/src/components/diff/file-block.tsx +++ b/packages/ui/src/components/diff/file-block.tsx @@ -72,6 +72,8 @@ interface FileBlockProps { onRefreshFile?: (path: string) => void; onAskThread?: (filePath: string, side: CommentSide, startLine: number, endLine: number, body: string, author: CommentAuthor) => void; onAskReply?: (threadId: string, body: string, author: CommentAuthor) => void; + onActThread?: (filePath: string, side: CommentSide, startLine: number, endLine: number, body: string, author: CommentAuthor) => void; + onActReply?: (threadId: string, body: string, author: CommentAuthor) => void; askIsHeard?: boolean; } @@ -87,7 +89,7 @@ export function FileBlock(props: FileBlockProps) { file, viewMode, collapsed, onToggleCollapse, reviewed, onReviewedChange, highlightLine, baseRef, canRevert, onRevert, focusRanges, tourMarks, activeStepIndex, onTourMarkClick, isStale, onRefreshFile, - onAskThread, onAskReply, askIsHeard, + onAskThread, onAskReply, onActThread, onActReply, askIsHeard, threads: allThreads, commentsEnabled, commentActions, onAddThread: rawAddThread, pendingSelection, onPendingSelectionChange, highlighted, onHighlightEnd, } = props; @@ -593,6 +595,8 @@ export function FileBlock(props: FileBlockProps) { attentionTitle={attentionTitle} onAskThread={onAskThread} onAskReply={onAskReply} + onActThread={onActThread} + onActReply={onActReply} askIsHeard={askIsHeard} tourMarks={tourMarks} activeStepIndex={activeStepIndex} diff --git a/packages/ui/src/components/diff/hunk-block-split.tsx b/packages/ui/src/components/diff/hunk-block-split.tsx index aa5a1ae..1382bd3 100644 --- a/packages/ui/src/components/diff/hunk-block-split.tsx +++ b/packages/ui/src/components/diff/hunk-block-split.tsx @@ -44,6 +44,8 @@ interface HunkBlockSplitProps { getOriginalCode?: (side: CommentSide, startLine: number, endLine: number) => string; onAskThread?: (filePath: string, side: CommentSide, startLine: number, endLine: number, body: string, author: CommentAuthor) => void; onAskReply?: (threadId: string, body: string, author: CommentAuthor) => void; + onActThread?: (filePath: string, side: CommentSide, startLine: number, endLine: number, body: string, author: CommentAuthor) => void; + onActReply?: (threadId: string, body: string, author: CommentAuthor) => void; askIsHeard?: boolean; tourMarks?: TourMark[]; activeStepIndex?: number; @@ -239,6 +241,7 @@ export function renderSplitRows( thread={thread} onReply={props.onReply!} onAskReply={props.onAskReply} + onActReply={props.onActReply} askIsHeard={props.askIsHeard} onResolve={props.onResolve!} onUnresolve={props.onUnresolve!} @@ -264,6 +267,7 @@ export function renderSplitRows( thread={thread} onReply={props.onReply!} onAskReply={props.onAskReply} + onActReply={props.onActReply} askIsHeard={props.askIsHeard} onResolve={props.onResolve!} onUnresolve={props.onUnresolve!} @@ -295,6 +299,7 @@ export function renderSplitRows( currentAuthor={props.currentAuthor} onSubmit={props.onAddThread} onAsk={props.onAskThread} + onAct={props.onActThread} askIsHeard={props.askIsHeard} onCancel={props.onCancelPending} viewMode="split" @@ -318,7 +323,7 @@ export function HunkBlockSplit(props: HunkBlockSplitProps) { onLineMouseDown, onLineMouseEnter, onCommentClick, onAddThread, onReply, onResolve, onUnresolve, onEditComment, onDeleteComment, onDeleteThread, onCancelPending, filePath, onRevertChange, getOriginalCode, - onAskThread, onAskReply, askIsHeard, + onAskThread, onAskReply, onActThread, onActReply, askIsHeard, tourMarks, activeStepIndex, onTourMarkClick, } = props; @@ -327,7 +332,7 @@ export function HunkBlockSplit(props: HunkBlockSplitProps) { threads, pendingSelection, currentAuthor, onAddThread, onReply, onResolve, onUnresolve, onEditComment, onDeleteComment, onDeleteThread, onCancelPending, filePath, getOriginalCode, - onAskThread, onAskReply, askIsHeard, + onAskThread, onAskReply, onActThread, onActReply, askIsHeard, tourMarks, activeStepIndex, onTourMarkClick, }; diff --git a/packages/ui/src/components/diff/hunk-block.tsx b/packages/ui/src/components/diff/hunk-block.tsx index b5a59ad..694e9f9 100644 --- a/packages/ui/src/components/diff/hunk-block.tsx +++ b/packages/ui/src/components/diff/hunk-block.tsx @@ -41,6 +41,8 @@ interface HunkBlockProps { getOriginalCode?: (side: CommentSide, startLine: number, endLine: number) => string; onAskThread?: (filePath: string, side: CommentSide, startLine: number, endLine: number, body: string, author: CommentAuthor) => void; onAskReply?: (threadId: string, body: string, author: CommentAuthor) => void; + onActThread?: (filePath: string, side: CommentSide, startLine: number, endLine: number, body: string, author: CommentAuthor) => void; + onActReply?: (threadId: string, body: string, author: CommentAuthor) => void; askIsHeard?: boolean; tourMarks?: TourMark[]; activeStepIndex?: number; @@ -88,6 +90,7 @@ export function renderLineWithComments( thread={thread} onReply={props.onReply!} onAskReply={props.onAskReply} + onActReply={props.onActReply} askIsHeard={props.askIsHeard} onResolve={props.onResolve!} onUnresolve={props.onUnresolve!} @@ -128,7 +131,7 @@ export function HunkBlock(props: HunkBlockProps) { onLineMouseDown, onLineMouseEnter, onCommentClick, onAddThread, onReply, onResolve, onUnresolve, onDeleteComment, onDeleteThread, onCancelPending, filePath, onRevertChange, getOriginalCode, - onAskThread, onAskReply, askIsHeard, + onAskThread, onAskReply, onActThread, onActReply, askIsHeard, tourMarks, activeStepIndex, onTourMarkClick, } = props; @@ -137,7 +140,7 @@ export function HunkBlock(props: HunkBlockProps) { threads, pendingSelection, currentAuthor, onAddThread, onReply, onResolve, onUnresolve, onDeleteComment, onDeleteThread, onCancelPending, filePath, getOriginalCode, - onAskThread, onAskReply, askIsHeard, + onAskThread, onAskReply, onActThread, onActReply, askIsHeard, tourMarks, activeStepIndex, onTourMarkClick, }; diff --git a/packages/ui/src/components/diff/hunk-with-gap.tsx b/packages/ui/src/components/diff/hunk-with-gap.tsx index 2d6016b..bc7ef4b 100644 --- a/packages/ui/src/components/diff/hunk-with-gap.tsx +++ b/packages/ui/src/components/diff/hunk-with-gap.tsx @@ -47,6 +47,8 @@ interface HunkWithGapProps { getOriginalCode?: (side: CommentSide, startLine: number, endLine: number) => string; onAskThread?: (filePath: string, side: CommentSide, startLine: number, endLine: number, body: string, author: CommentAuthor) => void; onAskReply?: (threadId: string, body: string, author: CommentAuthor) => void; + onActThread?: (filePath: string, side: CommentSide, startLine: number, endLine: number, body: string, author: CommentAuthor) => void; + onActReply?: (threadId: string, body: string, author: CommentAuthor) => void; askIsHeard?: boolean; tourMarks?: TourMark[]; activeStepIndex?: number; @@ -61,7 +63,7 @@ export function HunkWithGap(props: HunkWithGapProps) { onLineMouseDown, onLineMouseEnter, onCommentClick, onAddThread, onReply, onResolve, onUnresolve, onEditComment, onDeleteComment, onDeleteThread, onCancelPending, filePath, onRevertChange, getOriginalCode, - onAskThread, onAskReply, askIsHeard, + onAskThread, onAskReply, onActThread, onActReply, askIsHeard, tourMarks, activeStepIndex, onTourMarkClick, } = props; @@ -81,7 +83,7 @@ export function HunkWithGap(props: HunkWithGapProps) { threads, pendingSelection, currentAuthor, onAddThread, onReply, onResolve, onUnresolve, onEditComment, onDeleteComment, onDeleteThread, onCancelPending, filePath, - onAskThread, onAskReply, askIsHeard, + onAskThread, onAskReply, onActThread, onActReply, askIsHeard, tourMarks, activeStepIndex, onTourMarkClick, }; @@ -127,6 +129,8 @@ export function HunkWithGap(props: HunkWithGapProps) { getOriginalCode={getOriginalCode} onAskThread={onAskThread} onAskReply={onAskReply} + onActThread={onActThread} + onActReply={onActReply} askIsHeard={askIsHeard} tourMarks={tourMarks} activeStepIndex={activeStepIndex} diff --git a/packages/ui/src/components/icons/bell-icon.tsx b/packages/ui/src/components/icons/bell-icon.tsx new file mode 100644 index 0000000..8df637d --- /dev/null +++ b/packages/ui/src/components/icons/bell-icon.tsx @@ -0,0 +1,10 @@ +import type { SVGProps } from 'react'; + +export function BellIcon(props: SVGProps) { + return ( + + + + + ); +} diff --git a/packages/ui/src/components/layout/answer-bubble.tsx b/packages/ui/src/components/layout/answer-bubble.tsx index 0e956f5..b58623d 100644 --- a/packages/ui/src/components/layout/answer-bubble.tsx +++ b/packages/ui/src/components/layout/answer-bubble.tsx @@ -1,57 +1,94 @@ +import { useEffect, useRef, useState } from 'react'; import type { AnswerAlert } from '../../lib/answer-alerts'; import type { ThreadPosition } from '../../lib/thread-visibility'; import { XIcon } from '../icons/x-icon'; +/** Long enough to read three lines and decide, short enough not to become furniture. */ +export const SHOW_FOR_MS = 10_000; +const TICK_MS = 100; + interface AnswerBubbleProps { alerts: AnswerAlert[]; /** Which edge to sit on: the thread is behind the reader, or ahead of them. */ position: ThreadPosition; + /** Split has a middle gutter to sit left of; unified has one column and no midpoint. */ + viewMode: 'unified' | 'split'; onGo: (threadId: string) => void; + /** Its time ran out. The count that replaces it is the caller's business. */ + onExpire: () => void; onDismiss: () => void; } /** - * An answer arrived and the thread it belongs to is off screen. Sits on the edge the thread is on, - * so following it is a movement in the direction the bubble already suggests. - * - * Deliberately small and out of the way: a panel over the diff is worse than no panel, which the - * walkthrough tooltip taught the hard way. Several answers collapse into one — two bubbles competing - * for the same corner is worse than one that counts. + * Sits over the old side, on the edge the thread is on — never over the code being reviewed. Leaves + * on its own, with a bar running down so that is not a surprise. */ export function AnswerBubble(props: AnswerBubbleProps) { - const { alerts, position, onGo, onDismiss } = props; + const { alerts, position, viewMode, onGo, onExpire, onDismiss } = props; + const [remaining, setRemaining] = useState(1); + const newest = alerts[alerts.length - 1]; + const expiredRef = useRef(false); + + useEffect(() => { + if (!newest) { + return; + } + expiredRef.current = false; + setRemaining(1); + const startedAt = Date.now(); - if (alerts.length === 0) { + const timer = setInterval(() => { + const left = 1 - (Date.now() - startedAt) / SHOW_FOR_MS; + setRemaining(Math.max(0, left)); + if (left <= 0 && !expiredRef.current) { + expiredRef.current = true; + clearInterval(timer); + onExpire(); + } + }, TICK_MS); + + return () => clearInterval(timer); + }, [newest?.threadId, newest?.preview, onExpire]); + + if (!newest) { return null; } - const newest = alerts[alerts.length - 1]; const others = alerts.length - 1; const fileName = newest.filePath.split('/').pop() ?? newest.filePath; return (
- - +
+ + +
+
); } diff --git a/packages/ui/src/components/layout/live-indicator.tsx b/packages/ui/src/components/layout/live-indicator.tsx index 68e0f93..2882cdd 100644 --- a/packages/ui/src/components/layout/live-indicator.tsx +++ b/packages/ui/src/components/layout/live-indicator.tsx @@ -1,4 +1,5 @@ interface LiveIndicatorProps { + working: boolean; /** False when the server is not on a loopback bind, where live mode is refused outright. */ enabled: boolean; /** Whether an agent is parked on the claim route right now. */ @@ -12,7 +13,7 @@ interface LiveIndicatorProps { * three comments with silence because the mode was off and nothing said so. */ export function LiveIndicator(props: LiveIndicatorProps) { - const { enabled, listening, waiting } = props; + const { enabled, listening, working, waiting } = props; if (!enabled) { return null; @@ -21,16 +22,22 @@ export function LiveIndicator(props: LiveIndicatorProps) { return ( - - {listening ? 'Agent waiting' : 'No agent'} + + {working ? 'Agent working' : listening ? 'Agent waiting' : 'No agent'} {waiting > 0 && ( {waiting} diff --git a/packages/ui/src/components/layout/notification-bell.tsx b/packages/ui/src/components/layout/notification-bell.tsx new file mode 100644 index 0000000..f6297f8 --- /dev/null +++ b/packages/ui/src/components/layout/notification-bell.tsx @@ -0,0 +1,73 @@ +import { useEffect, useRef, useState } from 'react'; +import type { AnswerAlert } from '../../lib/answer-alerts'; +import { BellIcon } from '../icons/bell-icon'; + +interface NotificationBellProps { + alerts: AnswerAlert[]; + onGo: (threadId: string) => void; +} + +export function NotificationBell(props: NotificationBellProps) { + const { alerts, onGo } = props; + const [open, setOpen] = useState(false); + const rootRef = useRef(null); + + useEffect(() => { + if (!open) { + return; + } + const closeOnOutside = (event: MouseEvent) => { + if (!rootRef.current?.contains(event.target as Node)) { + setOpen(false); + } + }; + document.addEventListener('mousedown', closeOnOutside); + return () => document.removeEventListener('mousedown', closeOnOutside); + }, [open]); + + const label = alerts.length === 0 + ? 'No unread answers' + : `${alerts.length} unread answer${alerts.length === 1 ? '' : 's'}`; + + return ( +
+ + {open && ( +
+ {alerts.map(alert => ( + + ))} +
+ )} +
+ ); +} diff --git a/packages/ui/src/components/layout/toolbar.tsx b/packages/ui/src/components/layout/toolbar.tsx index 338103a..1ed0341 100644 --- a/packages/ui/src/components/layout/toolbar.tsx +++ b/packages/ui/src/components/layout/toolbar.tsx @@ -13,6 +13,8 @@ import { DiffStats } from '../diff/diff-stats'; import { GitHubDialog } from './github-dialog'; import { CommentToolbarActions } from '../comments/comment-toolbar-actions'; import { LiveIndicator } from './live-indicator'; +import { NotificationBell } from './notification-bell'; +import type { AnswerAlert } from '../../lib/answer-alerts'; import { OptionsMenu, menuItemClass } from './options-menu'; import { GENERAL_THREAD_FILE_PATH } from '../comments/types'; import type { ViewMode } from '../../lib/diff-utils'; @@ -21,7 +23,9 @@ import { isThreadResolved } from '../comments/types'; interface ToolbarProps { reviewInProgress?: boolean; - live?: { enabled: boolean; listening: boolean; waiting: number }; + live?: { enabled: boolean; listening: boolean; working: boolean; waiting: number }; + unreadAnswers?: AnswerAlert[]; + onGoToAnswer?: (threadId: string) => void; viewMode: ViewMode; onViewModeChange: (mode: ViewMode) => void; hideWhitespace: boolean; @@ -138,6 +142,8 @@ export function Toolbar(props: ToolbarProps) { githubDetails, reviewInProgress, live, + unreadAnswers, + onGoToAnswer, sessionId, onGitHubPulled, } = props; @@ -182,8 +188,16 @@ export function Toolbar(props: ToolbarProps) { )}
+ {onGoToAnswer && ( + + )} {live && ( - + )} { invalidateThreads(); }); diff --git a/packages/ui/src/lib/answer-alerts.ts b/packages/ui/src/lib/answer-alerts.ts index 930896a..be3664d 100644 --- a/packages/ui/src/lib/answer-alerts.ts +++ b/packages/ui/src/lib/answer-alerts.ts @@ -1,5 +1,6 @@ import type { CommentThread } from '../components/comments/types'; import { isAside } from './live-mode'; +import type { ThreadPosition } from './thread-visibility'; export interface AnswerAlert { threadId: string; @@ -22,7 +23,9 @@ export function newAnswers( previous: CommentThread[] | null, current: CommentThread[], ): AnswerAlert[] { - if (!previous) { + // An empty previous is a page that has not loaded its threads yet, not a review with no comments + // — announcing against it turns a reload into the whole conversation arriving at once. + if (!previous || previous.length === 0) { return []; } @@ -74,3 +77,26 @@ export function dropSeenAlerts( const kept = alerts.filter(alert => !isOnScreen(alert.threadId)); return kept.length === alerts.length ? alerts : kept; } + +/** A thread far from the reader is not rendered, so file order answers it when the DOM cannot. */ +export function positionForAlert( + alertFilePath: string, + activeFilePath: string | null, + orderedPaths: string[], + measured: ThreadPosition | null, +): ThreadPosition { + if (measured) { + return measured; + } + if (!activeFilePath) { + return 'below'; + } + + const alertAt = orderedPaths.indexOf(alertFilePath); + const readerAt = orderedPaths.indexOf(activeFilePath); + if (alertAt === -1 || readerAt === -1) { + return 'below'; + } + + return alertAt < readerAt ? 'above' : 'below'; +} diff --git a/packages/ui/src/lib/api.ts b/packages/ui/src/lib/api.ts index 31eacfa..449cb99 100644 --- a/packages/ui/src/lib/api.ts +++ b/packages/ui/src/lib/api.ts @@ -180,6 +180,7 @@ export function createThread(data: { /** An aside starts a conversation rather than a finding, and is never posted. */ kind?: CommentKind; live?: boolean; + intent?: 'ask' | 'act'; }): Promise { return apiFetch('/api/threads', { method: 'POST', @@ -193,6 +194,8 @@ export interface ReplyOptions { aside?: boolean; /** Ask the agent to answer, amend or act on it. */ live?: boolean; + /** A question, or a request for a change. Absent is a question. */ + intent?: 'ask' | 'act'; } export function replyToThread( @@ -209,6 +212,7 @@ export function replyToThread( author, kind: options.aside ? 'aside' : 'review', live: options.live === true, + intent: options.intent ?? 'ask', }), }); } diff --git a/packages/ui/src/lib/edit-box-size.ts b/packages/ui/src/lib/edit-box-size.ts new file mode 100644 index 0000000..8aa8b90 --- /dev/null +++ b/packages/ui/src/lib/edit-box-size.ts @@ -0,0 +1,13 @@ +export const MIN_EDIT_ROWS = 4; + +/** Roughly the line height at the size comments are set in, for turning rows into pixels. */ +const ROW_HEIGHT = 21; +const MAX_EDIT_ROWS = 24; + +export const MIN_EDIT_HEIGHT = MIN_EDIT_ROWS * ROW_HEIGHT; +export const MAX_EDIT_HEIGHT = MAX_EDIT_ROWS * ROW_HEIGHT; + +/** The element measures its own wrapped height; this only decides how far it may go. */ +export function clampEditHeight(measured: number): number { + return Math.min(MAX_EDIT_HEIGHT, Math.max(MIN_EDIT_HEIGHT, measured)); +} diff --git a/packages/ui/src/lib/live-mode.ts b/packages/ui/src/lib/live-mode.ts index 31630db..ee9f9b2 100644 --- a/packages/ui/src/lib/live-mode.ts +++ b/packages/ui/src/lib/live-mode.ts @@ -16,6 +16,28 @@ export function requestStateOf(comment: Comment): RequestState | null { return comment.liveClaimedAt ? 'working' : 'waiting'; } +export function intentOf(comment: Comment): 'ask' | 'act' { + return comment.liveIntent === 'act' ? 'act' : 'ask'; +} + export function isAside(comment: Comment): boolean { return comment.kind === 'aside'; } + +interface LiveStatusLike { + enabled: boolean; + mayChangeCode: boolean; +} + +export function canAskAgent(status: LiveStatusLike | undefined, reviewsEnabled: boolean): boolean { + return !!status?.enabled && reviewsEnabled; +} + +/** + * Acting is not. Reviewing is not editing, so a pull request somebody else wrote gets Ask and no + * Act — offered and then refused would be worse than not offered, since a button that is there is + * a promise. + */ +export function canActOnCode(status: LiveStatusLike | undefined, reviewsEnabled: boolean): boolean { + return canAskAgent(status, reviewsEnabled) && !!status?.mayChangeCode; +} diff --git a/packages/ui/src/queries/live.ts b/packages/ui/src/queries/live.ts index 1dc8af9..de1dfd6 100644 --- a/packages/ui/src/queries/live.ts +++ b/packages/ui/src/queries/live.ts @@ -4,7 +4,11 @@ import { apiFetch } from '../lib/api'; export interface LiveStatus { enabled: boolean; listening: boolean; + /** An agent has taken a request for this review and not answered it yet. */ + working: boolean; waiting: number; + /** Whether the agent may edit code here, derived from who wrote the pull request. */ + mayChangeCode: boolean; } /** diff --git a/packages/ui/tests/alert-position.test.ts b/packages/ui/tests/alert-position.test.ts new file mode 100644 index 0000000..2433999 --- /dev/null +++ b/packages/ui/tests/alert-position.test.ts @@ -0,0 +1,27 @@ +import { describe, it, expect } from 'vitest'; +import { positionForAlert } from '../src/lib/answer-alerts'; + +const order = ['a.ts', 'b.ts', 'c.ts', 'd.ts']; + +describe('which edge a note about an off-screen answer belongs on', () => { + it('trusts the measurement when the thread is rendered', () => { + expect(positionForAlert('d.ts', 'a.ts', order, 'above')).toBe('above'); + expect(positionForAlert('a.ts', 'd.ts', order, 'below')).toBe('below'); + }); + + // A thread far from the reader is not rendered at all, and null used to fall through to "below" — + // which is how a note about a thread above ended up in the bottom corner. + it('falls back to file order when the thread is not rendered', () => { + expect(positionForAlert('a.ts', 'c.ts', order, null)).toBe('above'); + expect(positionForAlert('d.ts', 'b.ts', order, null)).toBe('below'); + }); + + it('treats the same file as ahead, since the thread is further down it', () => { + expect(positionForAlert('b.ts', 'b.ts', order, null)).toBe('below'); + }); + + it('says below when it cannot tell', () => { + expect(positionForAlert('unknown.ts', 'b.ts', order, null)).toBe('below'); + expect(positionForAlert('a.ts', null, order, null)).toBe('below'); + }); +}); diff --git a/packages/ui/tests/answer-alerts.test.ts b/packages/ui/tests/answer-alerts.test.ts index 0dad4ef..00a5248 100644 --- a/packages/ui/tests/answer-alerts.test.ts +++ b/packages/ui/tests/answer-alerts.test.ts @@ -121,3 +121,21 @@ describe('dropping notes the reader has caught up with', () => { expect(dropSeenAlerts(alerts, () => true)).toEqual([]); }); }); + +describe('a page that has just been rebuilt', () => { + // Threads are an empty array while the query loads. Recording that as "what I have seen" makes + // every existing answer look new on the next poll — which is a page reload announcing the whole + // conversation back at you. + it('announces nothing when the first look was an empty load', () => { + const loaded = [thread('a', [{ id: 'c1', type: 'user' }, { id: 'c2', type: 'agent' }])]; + + expect(newAnswers([], loaded)).toEqual([]); + }); + + it('still announces an answer that arrives after a real look', () => { + const before = [thread('a', [{ id: 'c1', type: 'user' }])]; + const after = [thread('a', [{ id: 'c1', type: 'user' }, { id: 'c2', type: 'agent' }])]; + + expect(newAnswers(before, after)).toHaveLength(1); + }); +}); diff --git a/packages/ui/tests/answer-bubble.test.tsx b/packages/ui/tests/answer-bubble.test.tsx index 6070d3c..c05ef72 100644 --- a/packages/ui/tests/answer-bubble.test.tsx +++ b/packages/ui/tests/answer-bubble.test.tsx @@ -1,65 +1,97 @@ -import { describe, it, expect, afterEach, vi } from 'vitest'; -import { render, cleanup, screen } from '@testing-library/react'; -import { AnswerBubble } from '../src/components/layout/answer-bubble'; +import { describe, it, expect, afterEach, vi, beforeEach } from 'vitest'; +import { render, cleanup, screen, act } from '@testing-library/react'; +import { AnswerBubble, SHOW_FOR_MS } from '../src/components/layout/answer-bubble'; afterEach(cleanup); const one = [{ threadId: 't1', filePath: 'src/live.ts', authorName: 'Agent', preview: 'The stamp is written by the database.' }]; +function renderBubble(position: 'above' | 'below' = 'above', onExpire = vi.fn(), onGo = vi.fn()) { + render(); + return { onExpire, onGo }; +} + describe('an answer that arrived while you read on', () => { it('shows the reply and where it came from', () => { - render(); + renderBubble(); expect(screen.getByText(/The stamp is written by the database/)).toBeTruthy(); expect(screen.getByText(/live\.ts/)).toBeTruthy(); }); - it('sits at the top when the thread is behind you', () => { - render(); + // Over the old side, which is not the code the reader is reviewing. + // Right-aligned against the middle gutter, so it covers the right of the old side and never the + // new code being reviewed. + it('stops just short of the middle gutter in split view', () => { + renderBubble('above'); + const className = screen.getByRole('status').className; - expect(screen.getByRole('status').className).toContain('top-'); + expect(className).toContain('left-1/2'); + expect(className).toContain('-translate-x-full'); + expect(className).toContain('top-'); }); - it('sits at the bottom when the thread is ahead of you', () => { - render(); + it('sits low when the thread is ahead of you', () => { + renderBubble('below'); expect(screen.getByRole('status').className).toContain('bottom-'); }); - it('takes you there', () => { - const onGo = vi.fn(); - render(); - - screen.getByRole('button', { name: /the stamp is written/i }).click(); + it('keeps to the left in unified view, where there is no midpoint', () => { + render(); - expect(onGo).toHaveBeenCalledWith('t1'); + expect(screen.getByRole('status').className).toContain('left-4'); }); - it('can be sent away', () => { - const onDismiss = vi.fn(); - render(); + it('takes you there', () => { + const { onGo } = renderBubble(); - screen.getByRole('button', { name: /dismiss/i }).click(); + screen.getByRole('button', { name: /the stamp is written/i }).click(); - expect(onDismiss).toHaveBeenCalled(); + expect(onGo).toHaveBeenCalledWith('t1'); }); - // Two bubbles competing for the same corner is worse than one that counts. it('collapses several into one, showing the newest', () => { - const two = [ - one[0], - { threadId: 't2', filePath: 'src/agent.ts', authorName: 'Agent', preview: 'And the newest answer.' }, - ]; - render(); + render( + , + ); expect(screen.getAllByRole('status')).toHaveLength(1); expect(screen.getByText(/And the newest answer/)).toBeTruthy(); expect(screen.getByText(/1 more/)).toBeTruthy(); }); +}); + +describe('how long it stays', () => { + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); + + it('goes on its own rather than sitting there', () => { + const onExpire = vi.fn(); + render(); + + expect(onExpire).not.toHaveBeenCalled(); + act(() => void vi.advanceTimersByTime(SHOW_FOR_MS + 50)); + + expect(onExpire).toHaveBeenCalled(); + }); + + // The bar is the only thing saying it is about to leave, so it has to be running from the start. + it('shows how much time is left', () => { + render(); - it('is not there when nothing has arrived', () => { - render(); + const bar = screen.getByTestId('answer-bubble-timer'); + const atStart = bar.style.width; + act(() => void vi.advanceTimersByTime(SHOW_FOR_MS / 2)); - expect(screen.queryByRole('status')).toBeNull(); + expect(atStart).toBe('100%'); + expect(parseFloat(bar.style.width)).toBeLessThan(60); }); }); diff --git a/packages/ui/tests/ask-act-buttons.test.tsx b/packages/ui/tests/ask-act-buttons.test.tsx new file mode 100644 index 0000000..98046f1 --- /dev/null +++ b/packages/ui/tests/ask-act-buttons.test.tsx @@ -0,0 +1,43 @@ +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { render, cleanup, screen, fireEvent } from '@testing-library/react'; +import { CommentForm } from '../src/components/comments/comment-form'; + +afterEach(cleanup); + +describe('which of Ask and Act the reader is offered', () => { + // Reviewing is not editing. Act being absent on somebody else's pull request is the rule, and it + // is held up by a prop being undefined — exactly what a refactor breaks without noticing. + it('offers neither when no agent can be reached', () => { + render(); + + expect(screen.queryByRole('button', { name: 'Ask' })).toBeNull(); + expect(screen.queryByRole('button', { name: 'Act' })).toBeNull(); + }); + + it('offers Ask alone when the code is not the reader to change', () => { + render(); + + expect(screen.getByRole('button', { name: 'Ask' })).toBeTruthy(); + expect(screen.queryByRole('button', { name: 'Act' })).toBeNull(); + }); + + it('offers both on the reader own work', () => { + render(); + + expect(screen.getByRole('button', { name: 'Ask' })).toBeTruthy(); + expect(screen.getByRole('button', { name: 'Act' })).toBeTruthy(); + }); + + it('sends the text to whichever was pressed', () => { + const onAsk = vi.fn(); + const onAct = vi.fn(); + render(); + + // React tracks the value setter, so setting .value directly never reaches onChange. + fireEvent.change(screen.getByRole('textbox'), { target: { value: 'do the thing' } }); + screen.getByRole('button', { name: 'Act' }).click(); + + expect(onAct).toHaveBeenCalledWith('do the thing'); + expect(onAsk).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/ui/tests/can-act.test.ts b/packages/ui/tests/can-act.test.ts new file mode 100644 index 0000000..4b86d61 --- /dev/null +++ b/packages/ui/tests/can-act.test.ts @@ -0,0 +1,42 @@ +import { describe, it, expect } from 'vitest'; +import { canAskAgent, canActOnCode } from '../src/lib/live-mode'; + +describe('what the page offers', () => { + const listening = { enabled: true, listening: true, waiting: 0, mayChangeCode: true }; + + it('offers nothing when live mode is off', () => { + expect(canAskAgent({ ...listening, enabled: false }, true)).toBe(false); + expect(canActOnCode({ ...listening, enabled: false }, true)).toBe(false); + }); + + it('offers nothing when this diff has no review session', () => { + expect(canAskAgent(listening, false)).toBe(false); + expect(canActOnCode(listening, false)).toBe(false); + }); + + // Asking is always allowed where live mode is; acting is not. + it('lets the reader ask on somebody else pull request but not act', () => { + const someoneElses = { ...listening, mayChangeCode: false }; + + expect(canAskAgent(someoneElses, true)).toBe(true); + expect(canActOnCode(someoneElses, true)).toBe(false); + }); + + it('lets the reader do both on their own work', () => { + expect(canAskAgent(listening, true)).toBe(true); + expect(canActOnCode(listening, true)).toBe(true); + }); + + // Nobody listening is not a reason to hide the buttons: what you write is queued. + it('offers both even when nobody is listening yet', () => { + const quiet = { ...listening, listening: false }; + + expect(canAskAgent(quiet, true)).toBe(true); + expect(canActOnCode(quiet, true)).toBe(true); + }); + + it('offers nothing before the status has loaded', () => { + expect(canAskAgent(undefined, true)).toBe(false); + expect(canActOnCode(undefined, true)).toBe(false); + }); +}); diff --git a/packages/ui/tests/edit-box-size.test.ts b/packages/ui/tests/edit-box-size.test.ts new file mode 100644 index 0000000..d9ba315 --- /dev/null +++ b/packages/ui/tests/edit-box-size.test.ts @@ -0,0 +1,24 @@ +import { describe, it, expect } from 'vitest'; +import { clampEditHeight, MIN_EDIT_HEIGHT, MAX_EDIT_HEIGHT } from '../src/lib/edit-box-size'; + +describe('how tall the box for editing a comment may be', () => { + // A fixed three rows meant editing a long finding through a letterbox. + it('takes the measured height when it is reasonable', () => { + const middling = (MIN_EDIT_HEIGHT + MAX_EDIT_HEIGHT) / 2; + + expect(clampEditHeight(middling)).toBe(middling); + }); + + it('does not collapse for a one-line comment', () => { + expect(clampEditHeight(10)).toBe(MIN_EDIT_HEIGHT); + }); + + // Past a point the box would push the diff off screen, and scrolling inside it is the lesser evil. + it('stops growing before it takes over the page', () => { + expect(clampEditHeight(10_000)).toBe(MAX_EDIT_HEIGHT); + }); + + it('copes with a browser that measured nothing', () => { + expect(clampEditHeight(0)).toBe(MIN_EDIT_HEIGHT); + }); +}); diff --git a/packages/ui/tests/notification-bell.test.tsx b/packages/ui/tests/notification-bell.test.tsx new file mode 100644 index 0000000..54d0f98 --- /dev/null +++ b/packages/ui/tests/notification-bell.test.tsx @@ -0,0 +1,78 @@ +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { render, cleanup, screen, fireEvent } from '@testing-library/react'; +import { NotificationBell } from '../src/components/layout/notification-bell'; + +afterEach(cleanup); + +const two = [ + { threadId: 't1', filePath: 'packages/cli/src/live.ts', authorName: 'Agent', preview: 'first answer' }, + { threadId: 't2', filePath: 'packages/ui/src/api.ts', authorName: 'Agent', preview: 'second answer' }, +]; + +describe('the notification bell', () => { + it('is quiet with nothing unread', () => { + render(); + + expect(screen.queryByText('2')).toBeNull(); + expect(screen.getByRole('button', { name: /no unread/i })).toBeTruthy(); + }); + + it('counts what is unread', () => { + render(); + + expect(screen.getByText('2')).toBeTruthy(); + }); + + it('keeps the list closed until asked', () => { + render(); + + expect(screen.queryByText(/first answer/)).toBeNull(); + }); + + it('lists them oldest first, with where each came from', () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: /2 unread/i })); + + const items = screen.getAllByRole('menuitem'); + expect(items).toHaveLength(2); + expect(items[0].textContent).toContain('first answer'); + expect(items[0].textContent).toContain('live.ts'); + }); + + it('goes to the one that is clicked', () => { + const onGo = vi.fn(); + render(); + + fireEvent.click(screen.getByRole('button', { name: /2 unread/i })); + fireEvent.click(screen.getAllByRole('menuitem')[1]); + + expect(onGo).toHaveBeenCalledWith('t2'); + }); + + it('closes once you have gone somewhere', () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: /2 unread/i })); + fireEvent.click(screen.getAllByRole('menuitem')[0]); + + expect(screen.queryByRole('menuitem')).toBeNull(); + }); +}); + +describe('a long list', () => { + // A batch of asks is how this gets used — seven arrived in one go — and thirty would run off the + // bottom of the window with no way to reach the end. + it('scrolls inside itself rather than off the screen', () => { + const many = Array.from({ length: 30 }, (_, i) => ({ + threadId: `t${i}`, filePath: 'a.ts', authorName: 'Agent', preview: `answer ${i}`, + })); + render(); + + fireEvent.click(screen.getByRole('button', { name: /30 unread/i })); + + const list = screen.getAllByRole('menuitem')[0].parentElement!; + expect(list.className).toContain('max-h-'); + expect(list.className).toContain('overflow-y-auto'); + }); +}) diff --git a/skills/diffity-live/SKILL.md b/skills/diffity-live/SKILL.md index 124485e..13a2bd4 100644 --- a/skills/diffity-live/SKILL.md +++ b/skills/diffity-live/SKILL.md @@ -31,9 +31,35 @@ never did is guessing. `findingBody` is usually the thing being asked about. Read it before the question. -## Choose one of three +## Re-arm first -Read `body` and decide. Do not do more than was asked. +Before answering, put the loop back: + +``` +diffity agent await --timeout 240 +``` + +Background command, as always. The reader is looking at the page having just asked, and between +taking their request and answering it there is nobody parked — so the page said "No agent" at exactly +the moment they were watching. Re-arming first closes that window, and a second request arriving +while you write is queued rather than lost. + +Nothing about answering changes; it just happens second. + +## The reader already chose + +The request carries an `intent`, because there are two buttons in the page and they mean different +things. `await` prints what it means in plain words before the payload; that line is the instruction, +not a summary of it. + +- **`ask`** — a question. Answer it, or amend the finding it is about. **Do not change code**, however + obvious the change looks. They pressed Ask. +- **`act`** — a request for a change. Make it, and say what you did. + +An `intent` that is absent or unrecognised is a question. If `mayChangeCode` is false the answer is +the same whatever they pressed: this pull request is somebody else's. + +Read `body` and decide *how* to do what was asked. Do not do more than was asked. **Answer it.** A question about the finding, the code, or your reasoning. @@ -56,7 +82,8 @@ diffity agent reply --aside --answers --body "Rewritten Amend the **finding**, not the aside. If the finding has already been sent, `amend` tells you so — pass that on rather than letting the reader think the pull request has changed. -**Make the change.** Only on your own work. Read the rule below before you edit anything. +**Make the change.** Only when the intent is `act` and `mayChangeCode` is not false. Read the rule +below before you edit anything. ``` # make the edit, then: @@ -68,19 +95,21 @@ merge — those wait to be asked for, here as everywhere. ### When you must not change code -Run `diffity agent live-status` if you are unsure. Editing is off the table when the diff is +Run `diffity agent live-status` if you are unsure. + +Whether changes are allowed is decided when diffity is launched. `--review` says you are reviewing +somebody's change, `--work` says you are working on the branch — which is the case authorship gets +wrong, since taking over a colleague's branch leaves their name on the pull request. Said neither +way, it falls back to who wrote it. It lasts as long as that server and no longer, so a reader who +hands work over restarts rather than unticking something. Editing is off the table when the diff is somebody else's pull request, however the conversation goes: reviewing is not editing, and a reader asking a follow-up has not asked you to rewrite their branch. Answer and amend instead, and say that is what you did. -## Then go back to waiting - -``` -diffity agent await --timeout 240 -``` +## When the wait ends on its own -Background command, same as before. Exit 3 means nothing was asked — re-arm without saying anything. -Anything else means the server is gone; say so once and stop, rather than looping on a dead port. +Exit 3 means nothing was asked — re-arm without saying anything. Anything else means the server is +gone; say so once and stop, rather than looping on a dead port. ## Keep it short diff --git a/skills/diffity-review/SKILL.md b/skills/diffity-review/SKILL.md index 634e13e..f0ae9f5 100644 --- a/skills/diffity-review/SKILL.md +++ b/skills/diffity-review/SKILL.md @@ -100,7 +100,10 @@ The review needs a running session whose ref matches the requested ref. A ref mi - If refs **don't match** → restart: run `diffity --no-open --new` (or `diffity --no-open --new` if no ref). The `--new` flag kills the old session and starts a fresh one. Use Bash tool with `run_in_background: true`. Wait 2 seconds, then verify with `diffity list --json` and note the port. - If **no ref was requested** and the running session's ref is not `"work"` → restart with `diffity --no-open --new` (the running session is for a named ref, but we need working-tree). 3. If **no session is running** for this repo, start one in the background: - - Command: `diffity --no-open` (or `diffity --no-open` if no ref) + - Command: `diffity --no-open --review` (or `diffity --no-open --review` if no ref) + - `--review` says what you are here for. Reviewing is not editing, so it keeps Act off the + comment box and refuses a change request even on a pull request the reader wrote themselves. + Authorship alone cannot tell the difference; you can. - Use Bash tool with `run_in_background: true` - Wait 2 seconds, then verify with `diffity list --json` and note the port.