diff --git a/package-lock.json b/package-lock.json index a55a67b..7d7b277 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8452,7 +8452,7 @@ }, "packages/cli": { "name": "diffity", - "version": "0.9.13", + "version": "0.9.15", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8476,7 +8476,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.9.13", + "version": "0.9.15", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8485,7 +8485,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.9.13", + "version": "0.9.15", "dependencies": { "@diffity/parser": "*" }, @@ -8497,7 +8497,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.9.13", + "version": "0.9.15", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8505,7 +8505,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.9.13", + "version": "0.9.15", "dependencies": { "@react-router/node": "^7.13.2", "@tailwindcss/vite": "^4.2.1", diff --git a/packages/cli/package.json b/packages/cli/package.json index 90e91ca..3632baa 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "diffity", - "version": "0.9.13", + "version": "0.9.15", "description": "GitHub-style git diff viewer in the browser", "type": "module", "bin": { diff --git a/packages/cli/src/agent.ts b/packages/cli/src/agent.ts index 408de2e..32f4ed9 100644 --- a/packages/cli/src/agent.ts +++ b/packages/cli/src/agent.ts @@ -21,6 +21,7 @@ import { findInstanceForRepo, type RegistryEntry } from './registry.js'; import { createHash } from 'node:crypto'; import { createTour, addTourStep, updateTourStatus, deleteTour, deleteToursForSession, getTour } from './tours.js'; import { unansweredRequest } from './live-unanswered.js'; +import { describeSince, type SinceLastWait } from './live-events.js'; import { readAnchor, clampToFile, countWorkingTreeLines } from './anchor.js'; import { unescapeMarkdown as fromShell } from './unescape.js'; import { startReviewRun, finishReviewRun } from './review-run.js'; @@ -109,6 +110,10 @@ function formatThreadLine(thread: Thread): string { /** A `diffity agent await` that found nothing to do, told apart from one that failed. */ const NOTHING_ASKED_EXIT_CODE = 3; +/** Nobody has the review page open, so re-arming would wait for a question nobody can ask. */ +const NOBODY_WATCHING_EXIT_CODE = 4; +/** So the server does not count the agent's own polling as a window being open. */ +const AGENT_HEADER = { 'x-diffity-agent': '1' } as const; interface LiveStatus { available: boolean; @@ -337,23 +342,28 @@ Examples: ); } const startedAt = Date.now(); - let payload: { request: LiveRequest | null }; + let payload: { + request: LiveRequest | null; + since?: SinceLastWait; + viewerPresent?: boolean; + viewerGone?: boolean; + }; try { // 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 info = await fetch(`http://127.0.0.1:${instance.port}/api/info`, { headers: AGENT_HEADER }); 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' }); + const res = await fetch(claimUrl, { method: 'POST', headers: AGENT_HEADER }); if (!res.ok) { console.error(pc.red(`Could not wait for a request: ${res.status} ${await res.text()}`)); process.exitCode = 1; return; } - payload = (await res.json()) as { request: LiveRequest | null }; + payload = (await res.json()) as typeof payload; } catch (err) { // `fetch failed` on its own says nothing about why a held connection went away, and a // listener dying early is the failure that matters most here. The cause and how long it @@ -370,7 +380,21 @@ Examples: return; } + // Worth knowing, not worth waking for, so it is reported whatever else happened. + const missed = payload.since ? describeSince(payload.since) : null; + if (missed) { + console.error(pc.yellow(missed)); + } + if (!payload.request) { + // `viewerGone`, not `viewerPresent`: a page that has not been opened yet also has nobody + // watching, and stopping then would end the loop before the reader ever arrived. + if (payload.viewerGone) { + // Its own code so a loop can stop rather than re-arm into a closed window. + console.log(pc.dim('The review page was closed — stopping rather than waiting again')); + process.exitCode = NOBODY_WATCHING_EXIT_CODE; + return; + } // Its own code, so a loop can tell "nobody asked" from "something broke" and re-arm. console.log(pc.dim('Nothing was asked')); process.exitCode = NOTHING_ASKED_EXIT_CODE; diff --git a/packages/cli/src/db.ts b/packages/cli/src/db.ts index 2bc1bbe..01a1233 100644 --- a/packages/cli/src/db.ts +++ b/packages/cli/src/db.ts @@ -152,6 +152,9 @@ function migrateDb(db: DatabaseSync): void { // The wording that went out. Amending a finding rewrites its body in place, so without this there // is nothing left to recognise the forge's copy of it by. addColumn(db, 'comment_threads', 'submitted_body', 'TEXT'); + // When an agent last finished waiting on this session, so it can be told what happened while it + // was parked. Nothing wakes an agent for a submit, and it needs to know one happened. + addColumn(db, 'review_sessions', 'agent_seen_at', 'TEXT'); } function addColumn(db: DatabaseSync, table: string, column: string, type: string): void { diff --git a/packages/cli/src/live-events.ts b/packages/cli/src/live-events.ts new file mode 100644 index 0000000..ea4f3c0 --- /dev/null +++ b/packages/cli/src/live-events.ts @@ -0,0 +1,30 @@ +export interface SinceLastWait { + /** Findings that went to the forge while the agent was waiting. */ + submitted: number; +} + +/** + * What changed while the agent was parked, which it is otherwise never told. + * + * Submitting a review does not wake anything — the queue only carries what the reader asks — so an + * agent can answer a question about a finding that has already gone out and word it as though it + * had not. Carried on the way back rather than raised as an event: it is worth knowing, not worth + * interrupting for. + */ +export function sinceLastWait(submittedAt: (string | null)[], seenAt: string | null): SinceLastWait { + const after = seenAt ?? ''; + + return { + submitted: submittedAt.filter((at): at is string => !!at && at > after).length, + }; +} + +export function describeSince(since: SinceLastWait): string | null { + if (since.submitted === 0) { + return null; + } + const count = `${since.submitted} finding${since.submitted === 1 ? '' : 's'}`; + + return `${count} went to the pull request while you were waiting. Amending one now leaves the ` + + 'forge showing the old wording.'; +} diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index 9b30562..19c6661 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -47,7 +47,7 @@ import { type PrComment, type ReviewEvent, } from '@diffity/github'; -import { findOrCreateSession, resolveSessionId } from './session.js'; +import { findOrCreateSession, resolveSessionId, agentSeenAt, markAgentSeen } from './session.js'; import { resolveMayChangeCode, type SessionPurpose } from './live-permissions.js'; import { liveListenerCount, @@ -62,6 +62,8 @@ import { parseDiffStatSummary } from './diff-stat.js'; import { getReviewRun } from './review-run.js'; import { createThread, addReply, getThreadsForSession, markThreadsSubmitted, updateThreadStatus } from './threads.js'; import { threadsResolvedRemotely } from './github-resolution.js'; +import { noteViewerSeen, markViewerGone, viewerSnapshot, viewerIsPresent, viewerHasGone, VIEWER_POLL_MS } from './viewers.js'; +import { sinceLastWait } from './live-events.js'; import { handleReviewRoute } from './review-routes.js'; import { handleTourRoute } from './tour-routes.js'; import { sendJson, sendError, readBody } from './http-utils.js'; @@ -385,6 +387,22 @@ export function startServer(options: ServerOptions): Promise { return findOrCreateSession(url.searchParams.get('ref') || effectiveRef || 'work').id; }; + // The page says whether it is there, rather than being inferred from traffic it stops + // making: react query pauses polling on a hidden tab, so a window can be open and silent. + if (pathname === '/api/viewer' && req.method === 'POST') { + noteViewerSeen(); + sendJson(res, { ok: true }); + return; + } + + // Sent on `pagehide` as a beacon, so a closed tab is known at once instead of after the + // idle window has run out. + if (pathname === '/api/viewer/gone' && req.method === 'POST') { + markViewerGone(); + sendJson(res, { ok: true }); + return; + } + if (pathname === '/api/live/status') { const sid = liveSessionId(); sendJson(res, { @@ -393,6 +411,7 @@ export function startServer(options: ServerOptions): Promise { working: sid ? liveWorkingCount(sid) > 0 : false, waiting: sid ? pendingLiveCount(sid) : 0, mayChangeCode: resolveMayChangeCode(purpose, authorship()), + viewerPresent: viewerIsPresent(viewerSnapshot(), Date.now()), }); return; } @@ -419,21 +438,65 @@ export function startServer(options: ServerOptions): Promise { const listenerGone = new AbortController(); req.on('close', () => listenerGone.abort()); + // What happened while the last agent was parked, before this wait resets the watermark. + const since = sinceLastWait( + getThreadsForSession(sid).map(thread => thread.submittedAt), + agentSeenAt(sid), + ); + markAgentSeen(sid); + + // Nobody is going to ask anything through a window that is not open. Waiting anyway costs + // a parked request here and a re-arm every few minutes at the other end, forever. + // + // A window that has never been open is a different matter: an agent is usually armed + // before the reader opens the page, so that case waits. + if (viewerHasGone(viewerSnapshot(), Date.now())) { + sendJson(res, { request: null, since, viewerPresent: false, viewerGone: true }); + return; + } + // Set by the watcher below, read by the handler: `listenerGone` is aborted both when the + // agent hangs up and when the reader closes the page, and those need opposite responses — + // one has no socket left to write to, the other is waiting for an answer. + let endedBecauseViewerLeft = false; + const viewerWatch = setInterval(() => { + if (viewerHasGone(viewerSnapshot(), Date.now())) { + endedBecauseViewerLeft = true; + listenerGone.abort(); + } + }, VIEWER_POLL_MS); + const stopWatching = (): void => clearInterval(viewerWatch); + req.on('close', stopWatching); + waitForLiveRequest(sid, waitMs, listenerGone.signal).then( request => { + stopWatching(); // The connection may already be gone; writing to it would throw rather than help. - if (res.writableEnded || listenerGone.signal.aborted) { + if (res.writableEnded) { + return; + } + if (endedBecauseViewerLeft) { + sendJson(res, { request: null, since, viewerPresent: false, viewerGone: true }); + return; + } + if (listenerGone.signal.aborted) { return; } + const viewerPresent = viewerIsPresent(viewerSnapshot(), Date.now()); if (!request) { - sendJson(res, { request: null }); + sendJson(res, { request: null, since, viewerPresent, viewerGone: false }); return; } // 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: resolveMayChangeCode(purpose, authorship()) } }); + sendJson(res, { + request: { ...request, mayChangeCode: resolveMayChangeCode(purpose, authorship()) }, + since, + viewerPresent, + viewerGone: false, + }); }, err => { + stopWatching(); if (!res.writableEnded && !listenerGone.signal.aborted) { sendError(res, 500, `Failed to wait for a live request: ${err}`); } diff --git a/packages/cli/src/session.ts b/packages/cli/src/session.ts index 8ad4ae8..9947cb3 100644 --- a/packages/cli/src/session.ts +++ b/packages/cli/src/session.ts @@ -357,3 +357,19 @@ export function getCurrentSession(): Session | null { return null; } } + +/** When an agent last finished waiting on this session, or null if none ever has. */ +export function agentSeenAt(sessionId: string): string | null { + return queryOne<{ agent_seen_at: string | null }>( + 'SELECT agent_seen_at FROM review_sessions WHERE id = ?', + sessionId, + )?.agent_seen_at ?? null; +} + +export function markAgentSeen(sessionId: string): void { + getDb() + .prepare( + "UPDATE review_sessions SET agent_seen_at = strftime('%Y-%m-%d %H:%M:%f', 'now') WHERE id = ?", + ) + .run(sessionId); +} diff --git a/packages/cli/src/threads.ts b/packages/cli/src/threads.ts index ed2e688..7b14e91 100644 --- a/packages/cli/src/threads.ts +++ b/packages/cli/src/threads.ts @@ -147,7 +147,7 @@ export function markThreadsSubmitted( const db = getDb(); const statement = db.prepare( `UPDATE comment_threads - SET submitted_at = datetime('now'), + SET submitted_at = strftime('%Y-%m-%d %H:%M:%f', 'now'), submitted_review_url = ?, submitted_head_sha = ?, submitted_body = COALESCE(?, submitted_body) diff --git a/packages/cli/src/viewers.ts b/packages/cli/src/viewers.ts new file mode 100644 index 0000000..ceb315f --- /dev/null +++ b/packages/cli/src/viewers.ts @@ -0,0 +1,58 @@ +/** + * How long after the last sign of a page we still call somebody present. + * + * Generous because it is the fallback, not the main signal: a closed tab says so explicitly, and + * this only has to catch a crash or a kill. It also has to survive a hidden tab, where browsers + * throttle timers to roughly one a minute. + */ +export const VIEWER_IDLE_MS = 180_000; + +/** How often a wait re-checks whether the page is still there. */ +export const VIEWER_POLL_MS = 5_000; + +export interface ViewerState { + lastSeenAt: number; + /** Whether a page has ever been open, which is not the same as one being open now. */ + everSeen: boolean; +} + +let state: ViewerState = { lastSeenAt: 0, everSeen: false }; + +/** + * A page said it is there. Its own heartbeat, rather than any request it happens to make: react + * query stops polling a hidden tab, so ordinary traffic goes quiet while the window is still open. + */ +export function noteViewerSeen(now = Date.now()): void { + state = { lastSeenAt: now, everSeen: true }; +} + +/** The page said it is going away, which beats waiting for silence to prove it. */ +export function markViewerGone(): void { + state = { lastSeenAt: 0, everSeen: true }; +} + +export function viewerSnapshot(): ViewerState { + return state; +} + +export function viewerIsPresent(snapshot: ViewerState, now: number, idleMs = VIEWER_IDLE_MS): boolean { + if (snapshot.lastSeenAt === 0) { + return false; + } + return now - snapshot.lastSeenAt < idleMs; +} + +/** + * A window was open and is not any more — as distinct from one that has never been open. + * + * The difference decides whether waiting is pointless or merely early: an agent is usually armed + * before the reader opens the page, and stopping then would end the loop before it began. + */ +export function viewerHasGone(snapshot: ViewerState, now: number, idleMs = VIEWER_IDLE_MS): boolean { + return snapshot.everSeen && !viewerIsPresent(snapshot, now, idleMs); +} + +/** Only used by tests, which would otherwise inherit whatever the last one left behind. */ +export function resetViewerSeen(): void { + state = { lastSeenAt: 0, everSeen: false }; +} diff --git a/packages/cli/tests/live-events.test.ts b/packages/cli/tests/live-events.test.ts new file mode 100644 index 0000000..053d34e --- /dev/null +++ b/packages/cli/tests/live-events.test.ts @@ -0,0 +1,35 @@ +import { describe, it, expect } from 'vitest'; +import { sinceLastWait, describeSince } from '../src/live-events.js'; + +const t = (s: string) => `2026-08-26 12:${s}:00`; + +describe('sinceLastWait', () => { + it('counts what went out after the agent last looked', () => { + expect(sinceLastWait([t('05'), t('15')], t('10')).submitted).toBe(1); + }); + + it('counts nothing when nothing went out', () => { + expect(sinceLastWait([null, null], t('10')).submitted).toBe(0); + }); + + // First wait of a session: everything already sent is news, because the agent has not looked yet. + it('counts everything when the agent has never looked', () => { + expect(sinceLastWait([t('05'), t('15')], null).submitted).toBe(2); + }); + + it('does not count something sent at the exact moment it looked', () => { + expect(sinceLastWait([t('10')], t('10')).submitted).toBe(0); + }); +}); + +describe('describeSince', () => { + it('says nothing when nothing happened', () => { + expect(describeSince({ submitted: 0 })).toBeNull(); + }); + + it('warns about amending, which is the reason this matters', () => { + expect(describeSince({ submitted: 1 })).toContain('1 finding went to the pull request'); + expect(describeSince({ submitted: 1 })).toContain('old wording'); + expect(describeSince({ submitted: 3 })).toContain('3 findings'); + }); +}); diff --git a/packages/cli/tests/liveness-gone.test.ts b/packages/cli/tests/liveness-gone.test.ts new file mode 100644 index 0000000..d59d9ee --- /dev/null +++ b/packages/cli/tests/liveness-gone.test.ts @@ -0,0 +1,62 @@ +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; +let port: number; +let close: () => void; + +const AGENT = { 'x-diffity-agent': '1' }; + +beforeAll(async () => { + origCwd = process.cwd(); + root = mkdtempSync(join(tmpdir(), 'diffity-gone-')); + 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); + const { startServer } = await import('../src/server.js'); + const started = await startServer({ port: 0, diffArgs: [], effectiveRef: 'work' }); + port = started.port; + close = started.close; +}); + +afterAll(() => { + close?.(); + process.chdir(origCwd); + delete process.env.DIFFITY_DATA_DIR; + rmSync(root, { recursive: true, force: true }); +}); + +// The whole point: a window was open, the reader closed the tab, and the next wait stops instead of +// parking for a question nobody can now ask. +describe('a window that was open and has gone', () => { + it('is told apart from one that never opened, and stops the wait', async () => { + const { noteViewerSeen, VIEWER_IDLE_MS } = await import('../src/viewers.js'); + + // A page was open, long enough ago that its heartbeat has clearly stopped. + noteViewerSeen(Date.now() - VIEWER_IDLE_MS - 1); + + const started = Date.now(); + const res = await fetch(`http://127.0.0.1:${port}/api/live/claim?wait=30`, { + method: 'POST', + headers: AGENT, + }); + const body = await res.json(); + + expect(body.viewerPresent).toBe(false); + expect(body.request).toBeNull(); + expect(Date.now() - started).toBeLessThan(3_000); + }); +}); diff --git a/packages/cli/tests/liveness-routes.test.ts b/packages/cli/tests/liveness-routes.test.ts new file mode 100644 index 0000000..e77ad7f --- /dev/null +++ b/packages/cli/tests/liveness-routes.test.ts @@ -0,0 +1,134 @@ +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; +let port: number; +let close: () => void; + +const AGENT = { 'x-diffity-agent': '1' }; + +async function req(path: string, init: RequestInit = {}): Promise<{ status: number; body: any }> { + const res = await fetch(`http://127.0.0.1:${port}${path}`, init); + const text = await res.text(); + return { status: res.status, body: text ? JSON.parse(text) : null }; +} + +/** What the page's heartbeat does. Ordinary traffic no longer counts, since a hidden tab makes none. */ +function pageIsOpen(): Promise<{ status: number; body: any }> { + return req('/api/viewer', { method: 'POST' }); +} + +/** The claim route, as the agent calls it: saying who it is, so it is not mistaken for a window. */ +function claim(waitSeconds: number): Promise<{ status: number; body: any }> { + return req(`/api/live/claim?wait=${waitSeconds}`, { method: 'POST', headers: AGENT }); +} + +beforeAll(async () => { + origCwd = process.cwd(); + root = mkdtempSync(join(tmpdir(), 'diffity-liveness-')); + 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); + const { startServer } = await import('../src/server.js'); + const started = await startServer({ port: 0, diffArgs: [], effectiveRef: 'work' }); + port = started.port; + close = started.close; +}); + +afterAll(() => { + close?.(); + process.chdir(origCwd); + delete process.env.DIFFITY_DATA_DIR; + rmSync(root, { recursive: true, force: true }); +}); + +describe('waiting when nobody is watching', () => { + // An agent is usually armed before the reader opens the page, so a window that has never been + // open means waiting is early rather than pointless. + it('still waits when no page has ever been open', async () => { + const started = Date.now(); + const { body } = await claim(2); + + expect(body.request).toBeNull(); + expect(body.viewerPresent).toBe(false); + expect(Date.now() - started).toBeGreaterThanOrEqual(1_500); + }); + + it('parks once a page has been seen', async () => { + await pageIsOpen(); + + const started = Date.now(); + const { body } = await claim(2); + + expect(body.viewerPresent).toBe(true); + expect(Date.now() - started).toBeGreaterThanOrEqual(1_500); + }); + + // Silence is not absence: react query stops polling a hidden tab, so a window can be open and + // making no requests at all. Only the heartbeat counts. + it('does not take ordinary traffic as proof a window is open', async () => { + const { resetViewerSeen } = await import('../src/viewers.js'); + resetViewerSeen(); + + await req('/api/info'); + const { body } = await claim(0); + + expect(body.viewerPresent).toBe(false); + expect(body.viewerGone).toBe(false); + }); + + // The precise signal, which is what the whole thing turns on. + it('knows at once when the page says it is closing', async () => { + await pageIsOpen(); + expect((await claim(0)).body.viewerPresent).toBe(true); + + await req('/api/viewer/gone', { method: 'POST' }); + + const { body } = await claim(30); + expect(body.viewerGone).toBe(true); + expect(body.viewerPresent).toBe(false); + }); +}); + +describe('what the agent missed while it was parked', () => { + it('reports nothing on a quiet session', async () => { + await pageIsOpen(); + const { body } = await claim(0); + + expect(body.since).toEqual({ submitted: 0 }); + }); + + it('reports a finding that went to the forge, once', async () => { + await pageIsOpen(); + const { getCurrentSession } = await import('../src/session.js'); + const { createThread, markThreadsSubmitted } = await import('../src/threads.js'); + + const session = getCurrentSession()!; + const thread = createThread(session.id, 'a.ts', 'new', 1, 1, 'P2: a finding', { + name: 'Agent', + type: 'agent', + }); + markThreadsSubmitted([{ threadId: thread.id, body: 'P2: a finding' }]); + + const first = await claim(0); + expect(first.body.since.submitted).toBe(1); + + // The watermark moved, so the same submit is not reported twice. + await pageIsOpen(); + const second = await claim(0); + expect(second.body.since.submitted).toBe(0); + }); +}); diff --git a/packages/cli/tests/server-routes.test.ts b/packages/cli/tests/server-routes.test.ts index b75ed1e..a02b6c9 100644 --- a/packages/cli/tests/server-routes.test.ts +++ b/packages/cli/tests/server-routes.test.ts @@ -200,7 +200,7 @@ describe('the live loop', () => { const claim = await req('/api/live/claim?wait=0', { method: 'POST' }); expect(claim.status).toBe(200); - expect(JSON.parse(claim.text)).toEqual({ request: null }); + expect(JSON.parse(claim.text)).toMatchObject({ request: null }); }); it('hands over an aside that asked for the agent', async () => { @@ -237,7 +237,7 @@ describe('the live loop', () => { const claim = await req('/api/live/claim?wait=0', { method: 'POST' }); - expect(JSON.parse(claim.text)).toEqual({ request: null }); + expect(JSON.parse(claim.text)).toMatchObject({ request: null }); }); // Claiming mutates, so it must be a write as far as the guard is concerned. @@ -294,7 +294,7 @@ describe('a listener that waits', () => { const elapsed = Date.now() - before; expect(claim.status).toBe(200); - expect(JSON.parse(claim.text)).toEqual({ request: null }); + expect(JSON.parse(claim.text)).toMatchObject({ request: null }); expect(elapsed).toBeGreaterThanOrEqual(900); }); }); diff --git a/packages/cli/tests/viewers.test.ts b/packages/cli/tests/viewers.test.ts new file mode 100644 index 0000000..9f25cb8 --- /dev/null +++ b/packages/cli/tests/viewers.test.ts @@ -0,0 +1,72 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { + noteViewerSeen, + markViewerGone, + viewerSnapshot, + viewerIsPresent, + viewerHasGone, + resetViewerSeen, + VIEWER_IDLE_MS, +} from '../src/viewers.js'; + +beforeEach(resetViewerSeen); + +describe('viewerIsPresent', () => { + it('is nobody until a page has said it is there', () => { + expect(viewerIsPresent(viewerSnapshot(), 1_000)).toBe(false); + }); + + it('is somebody just after a heartbeat', () => { + noteViewerSeen(1_000); + + expect(viewerIsPresent(viewerSnapshot(), 1_000)).toBe(true); + }); + + // The window has to be wide enough for a hidden tab, whose timers a browser throttles. + it('is still somebody inside the idle window', () => { + noteViewerSeen(1_000); + + expect(viewerIsPresent(viewerSnapshot(), 1_000 + VIEWER_IDLE_MS - 1)).toBe(true); + }); + + it('is nobody once the heartbeat stops', () => { + noteViewerSeen(1_000); + + expect(viewerIsPresent(viewerSnapshot(), 1_000 + VIEWER_IDLE_MS)).toBe(false); + }); +}); + +describe('viewerHasGone', () => { + it('is false when no window has ever been open, because waiting is early not pointless', () => { + expect(viewerHasGone(viewerSnapshot(), 1_000_000)).toBe(false); + }); + + it('is false while a window is still beating', () => { + noteViewerSeen(1_000); + + expect(viewerHasGone(viewerSnapshot(), 1_000)).toBe(false); + }); + + it('is true once one that was open falls silent', () => { + noteViewerSeen(1_000); + + expect(viewerHasGone(viewerSnapshot(), 1_000 + VIEWER_IDLE_MS)).toBe(true); + }); + + // The point of the explicit signal: a closed tab is known at once rather than after three + // minutes of silence, which is the difference between a loop that stops and one that lingers. + it('is true immediately when the page says it is going', () => { + noteViewerSeen(1_000); + markViewerGone(); + + expect(viewerHasGone(viewerSnapshot(), 1_001)).toBe(true); + expect(viewerIsPresent(viewerSnapshot(), 1_001)).toBe(false); + }); + + it('is false again when the page comes back', () => { + markViewerGone(); + noteViewerSeen(2_000); + + expect(viewerHasGone(viewerSnapshot(), 2_000)).toBe(false); + }); +}); diff --git a/packages/git/package.json b/packages/git/package.json index dc4d82c..84e391a 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.9.13", + "version": "0.9.15", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/package.json b/packages/github/package.json index 439a461..1100600 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.9.13", + "version": "0.9.15", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/parser/package.json b/packages/parser/package.json index 77d00aa..cbcf4fb 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.9.13", + "version": "0.9.15", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/skills/diffity-live/SKILL.md b/packages/skills/diffity-live/SKILL.md index f884b71..7556a67 100644 --- a/packages/skills/diffity-live/SKILL.md +++ b/packages/skills/diffity-live/SKILL.md @@ -108,8 +108,20 @@ is what you did. ## When the wait ends on its own -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. + +Exit 4 means the review page has been closed. Stop; do not re-arm. Nobody can ask anything through a +window that is not open, and each re-arm costs a turn. Say in one line that the loop has ended, so +the reader knows to say the word if they open it again. + +Anything else means the server is gone; say so once and stop, rather than looping on a dead port. + +## What you may have missed + +`await` reports, on the way back, anything that happened while you were parked — currently whether +findings went to the pull request. Read it before answering: a question about a finding that has +already been sent is a different question, because amending it now leaves the forge showing the old +wording. Say so rather than quietly amending. ## Keep it short diff --git a/packages/ui/package.json b/packages/ui/package.json index 1ac3bf2..6c4836d 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.9.13", + "version": "0.9.15", "type": "module", "private": true, "scripts": { diff --git a/packages/ui/src/components/diff/diff-page.tsx b/packages/ui/src/components/diff/diff-page.tsx index e27a356..04ec311 100644 --- a/packages/ui/src/components/diff/diff-page.tsx +++ b/packages/ui/src/components/diff/diff-page.tsx @@ -4,6 +4,7 @@ import { useQuery, useQueryClient } from '@tanstack/react-query'; import { useDiff } from '../../hooks/use-diff'; import { useInfo } from '../../hooks/use-info'; import { useTheme } from '../../hooks/use-theme'; +import { useViewerPresence } from '../../hooks/use-viewer-presence'; import { useWrapLines } from '../../hooks/use-wrap-lines'; import { useKeyboard } from '../../hooks/use-keyboard'; import { useReviewThreads } from '../../hooks/use-review-threads'; @@ -56,6 +57,10 @@ export function DiffPage() { view: 'split' | 'unified' | null; }>(); + // An agent waiting for a question cannot see this window any other way: the page holds no + // connection, and its polling stops while the tab is hidden. + useViewerPresence(true); + const [viewMode, setViewMode] = useState(initialViewMode || 'split'); const { hideWhitespace, setHideWhitespace } = useHideWhitespace(); const [showHelp, setShowHelp] = useState(false); diff --git a/packages/ui/src/hooks/use-viewer-presence.ts b/packages/ui/src/hooks/use-viewer-presence.ts new file mode 100644 index 0000000..f15bafe --- /dev/null +++ b/packages/ui/src/hooks/use-viewer-presence.ts @@ -0,0 +1,44 @@ +import { useEffect } from 'react'; + +const BEAT_MS = 15_000; + +/** + * Tells the server this window is open, and tells it once when it closes. + * + * An agent waiting for a question has no other way to know: the page holds no connection, and its + * ordinary polling stops while the tab is hidden, so silence does not mean the window is gone. The + * closing message goes by `sendBeacon`, which is delivered during unload where a normal request is + * abandoned. + */ +export function useViewerPresence(enabled: boolean): void { + useEffect(() => { + if (!enabled) { + return; + } + + const beat = (): void => { + void fetch('/api/viewer', { method: 'POST', keepalive: true }).catch(() => {}); + }; + + beat(); + const timer = setInterval(beat, BEAT_MS); + // A hidden tab has its timers throttled, so say so again as soon as it is looked at. + const onVisible = (): void => { + if (document.visibilityState === 'visible') { + beat(); + } + }; + const onHide = (): void => { + navigator.sendBeacon?.('/api/viewer/gone'); + }; + + document.addEventListener('visibilitychange', onVisible); + window.addEventListener('pagehide', onHide); + + return () => { + clearInterval(timer); + document.removeEventListener('visibilitychange', onVisible); + window.removeEventListener('pagehide', onHide); + }; + }, [enabled]); +} diff --git a/packages/ui/tests/viewer-presence.test.tsx b/packages/ui/tests/viewer-presence.test.tsx new file mode 100644 index 0000000..6045cd7 --- /dev/null +++ b/packages/ui/tests/viewer-presence.test.tsx @@ -0,0 +1,67 @@ +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { render, cleanup, act } from '@testing-library/react'; +import { useViewerPresence } from '../src/hooks/use-viewer-presence'; + +function Page() { + useViewerPresence(true); + return null; +} + +afterEach(() => { + cleanup(); + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +describe('useViewerPresence', () => { + it('says the window is open straight away, and keeps saying it', () => { + vi.useFakeTimers(); + vi.stubGlobal('fetch', vi.fn(() => Promise.resolve(new Response('{}')))); + + render(); + expect(fetch).toHaveBeenCalledWith('/api/viewer', expect.objectContaining({ method: 'POST' })); + + const first = (fetch as unknown as { mock: { calls: unknown[] } }).mock.calls.length; + act(() => void vi.advanceTimersByTime(30_000)); + expect((fetch as unknown as { mock: { calls: unknown[] } }).mock.calls.length).toBeGreaterThan(first); + }); + + // A hidden tab has its timers throttled, so the heartbeat alone can go stale. + it('says so again as soon as the tab is looked at', () => { + vi.stubGlobal('fetch', vi.fn(() => Promise.resolve(new Response('{}')))); + render(); + const before = (fetch as unknown as { mock: { calls: unknown[] } }).mock.calls.length; + + act(() => { + document.dispatchEvent(new Event('visibilitychange')); + }); + + expect((fetch as unknown as { mock: { calls: unknown[] } }).mock.calls.length).toBe(before + 1); + }); + + // The whole point: closing the tab is known at once rather than inferred from silence. + it('sends a beacon when the page goes away', () => { + const sendBeacon = vi.fn(() => true); + vi.stubGlobal('fetch', vi.fn(() => Promise.resolve(new Response('{}')))); + vi.stubGlobal('navigator', { ...navigator, sendBeacon }); + + render(); + act(() => { + window.dispatchEvent(new Event('pagehide')); + }); + + expect(sendBeacon).toHaveBeenCalledWith('/api/viewer/gone'); + }); + + it('stops beating when the page unmounts', () => { + vi.useFakeTimers(); + vi.stubGlobal('fetch', vi.fn(() => Promise.resolve(new Response('{}')))); + + const { unmount } = render(); + unmount(); + const after = (fetch as unknown as { mock: { calls: unknown[] } }).mock.calls.length; + act(() => void vi.advanceTimersByTime(60_000)); + + expect((fetch as unknown as { mock: { calls: unknown[] } }).mock.calls.length).toBe(after); + }); +}); diff --git a/skills/diffity-live/SKILL.md b/skills/diffity-live/SKILL.md index 13a2bd4..3781b2a 100644 --- a/skills/diffity-live/SKILL.md +++ b/skills/diffity-live/SKILL.md @@ -108,8 +108,20 @@ is what you did. ## When the wait ends on its own -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. + +Exit 4 means the review page has been closed. Stop; do not re-arm. Nobody can ask anything through a +window that is not open, and each re-arm costs a turn. Say in one line that the loop has ended, so +the reader knows to say the word if they open it again. + +Anything else means the server is gone; say so once and stop, rather than looping on a dead port. + +## What you may have missed + +`await` reports, on the way back, anything that happened while you were parked — currently whether +findings went to the pull request. Read it before answering: a question about a finding that has +already been sent is a different question, because amending it now leaves the forge showing the old +wording. Say so rather than quietly amending. ## Keep it short