Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/cli/package.json
Original file line numberDiff line numberDiff line change
@@ -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": {
Expand Down
32 changes: 28 additions & 4 deletions packages/cli/src/agent.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand DownExpand Up@@ -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;
Expand DownExpand Up@@ -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
Expand All@@ -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;
Expand Down
3 changes: 3 additions & 0 deletions packages/cli/src/db.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 {
Expand Down
30 changes: 30 additions & 0 deletions packages/cli/src/live-events.ts
Original file line numberDiff line numberDiff line change
@@ -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.';
}
71 changes: 67 additions & 4 deletions packages/cli/src/server.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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,
Expand All@@ -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';
Expand DownExpand Up@@ -385,6 +387,22 @@ export function startServer(options: ServerOptions): Promise<ServerResult> {
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, {
Expand All@@ -393,6 +411,7 @@ export function startServer(options: ServerOptions): Promise<ServerResult> {
working: sid ? liveWorkingCount(sid) > 0 : false,
waiting: sid ? pendingLiveCount(sid) : 0,
mayChangeCode: resolveMayChangeCode(purpose, authorship()),
viewerPresent: viewerIsPresent(viewerSnapshot(), Date.now()),
});
return;
}
Expand All@@ -419,21 +438,65 @@ export function startServer(options: ServerOptions): Promise<ServerResult> {
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}`);
}
Expand Down
16 changes: 16 additions & 0 deletions packages/cli/src/session.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
}
2 changes: 1 addition & 1 deletion packages/cli/src/threads.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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)
Expand Down
58 changes: 58 additions & 0 deletions packages/cli/src/viewers.ts
Original file line numberDiff line numberDiff line change
@@ -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 };
}
Loading