From 0c2439bfe9b176a328d4532e9da87f174e6526b1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 26 Oct 2026 13:04:36 +0000 Subject: [PATCH 1/5] Announce plugin and AI results as they change; the web client follows them The server now sends two notifications to a subscribed client, through the Notifier PR 303 added: - Server.PluginOutputChanged, each time a plugin's stored result for a PR is written (pending, success, error, deferred) or cleared for a rerun. Every write goes through storePluginResult, which notifies after the upsert, and RerunPlugins announces each result it clears before it replies. - Server.AIOutputChanged, as an AI run is claimed (pending) and as its result is stored. The ai package can't import server, so the Runner gets an OnChange hook, which RunServer points at the notifier. Both name the PR and the plugin or feature with its new status, never the output, so a client showing it reads GetPluginOutput / GetAIOutput again. The web client stops polling for either. A shared useServerChanges hook re-reads when an announcement names the PR being shown, and when the /api/events stream reconnects (ServerEvents.onOpen), since anything sent while it was down is lost. Reads go one at a time, a burst asking for one more. usePluginOutputs, useAIReport and useAIBackgroundRuns use it in place of their poll loops; useAIReport applies only a reply newer than the one it shows, so a run reply written before the run landed can't overwrite it. The "still running, check back later" notice goes, along with the poll timings. The e2e fake now runs plugin reruns and AI runs on timers and announces each step, as the server does, rather than landing them after so many reads. New specs check that nothing is read while a held run is pending, and that runs the page never asked for show up. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011prZsf8nHYe7e2Ke1rrq6z --- ai/runner.go | 26 ++- ai/runner_test.go | 62 ++++++ bun_client/e2e/README.md | 12 +- bun_client/e2e/ai_report.e2e.ts | 39 +++- bun_client/e2e/harness/fake_crs.ts | 199 +++++++++++------ bun_client/e2e/harness/test.ts | 28 +++ bun_client/e2e/pr_list.e2e.ts | 24 ++- bun_client/e2e/review.e2e.ts | 14 +- bun_client/frontend/src/ai_utils.test.ts | 5 - bun_client/frontend/src/ai_utils.ts | 8 - bun_client/frontend/src/api.ts | 3 +- .../frontend/src/components/AIOutput.tsx | 5 +- .../frontend/src/components/AIReportModal.tsx | 5 +- .../frontend/src/components/AIReportView.tsx | 7 - bun_client/frontend/src/components/Review.tsx | 3 +- .../frontend/src/hooks/useAIBackgroundRuns.ts | 79 ++++--- bun_client/frontend/src/hooks/useAIReport.ts | 114 +++++----- .../frontend/src/hooks/usePluginOutputs.ts | 77 +++---- .../frontend/src/hooks/useServerChanges.ts | 72 +++++++ bun_client/frontend/src/plugin_utils.test.ts | 18 -- bun_client/frontend/src/plugin_utils.ts | 19 +- .../frontend/src/server_events.test.tsx | 60 ++++++ bun_client/frontend/src/server_events.ts | 90 ++++++++ docs/ai_features.md | 5 +- docs/protocol.md | 50 ++++- docs/web_client_features.md | 44 ++-- server/ai_test.go | 66 ++++++ server/notify.go | 80 ++++++- server/plugins.go | 19 +- server/plugins_test.go | 200 ++++++++++++++++++ server/server.go | 14 +- 31 files changed, 1151 insertions(+), 296 deletions(-) create mode 100644 bun_client/frontend/src/hooks/useServerChanges.ts diff --git a/ai/runner.go b/ai/runner.go index 2cf61636..0109ecd3 100644 --- a/ai/runner.go +++ b/ai/runner.go @@ -37,6 +37,13 @@ type Runner struct { // NewProvider builds the provider a run's config chose. Tests replace it. NewProvider func(choice config.AIProviderChoice) (Provider, error) + // OnChange, when set, is told each time a feature's output for a PR + // changes as a read of it would see: a run is claimed (pending, queued + // for a slot included) and the run's result is stored (its status). It is + // called on the claiming caller's goroutine or the run's, and must not + // block. The server sets it to notify subscribed clients. + OnChange func(owner, repo string, number int, feature, status string) + slots chan struct{} inflight sync.Map // runKey -> struct{} } @@ -152,6 +159,7 @@ func (r *Runner) claim(job Job) (claim, Outcome) { if _, running := r.inflight.LoadOrStore(runKey(job.Owner, job.Repo, job.Number, job.Feature), struct{}{}); running { return claim{}, OutcomeAlreadyRunning } + r.changed(job, StatusPending) return claim{job: job, feature: f, mode: resolveMode(f, entry), provider: cfg.AIProviderFor(entry)}, OutcomeStarted } @@ -227,10 +235,11 @@ func (r *Runner) execute(c claim) { BodyContent: fmt.Sprintf("**%s failed.**\n\n%v", c.feature.Name(), err), }} } - store(job, status, result, sha, digest) + status = store(job, status, result, sha, digest) // The result is readable from here on; don't keep reporting it as pending // while the log entry is written. r.inflight.Delete(key) + r.changed(job, status) entry := llm.CallReport{ Title: "AI Feature Run", @@ -301,11 +310,19 @@ func runFeature(ctx context.Context, f Feature, req Request) (res Result, err er return f.Run(ctx, req) } -// store writes a run's result, keyed by the inputs it was computed from. -func store(job Job, status string, result Result, sha, digest string) { +// changed tells OnChange, if set, that job's feature output now reads status. +func (r *Runner) changed(job Job, status string) { + if r.OnChange != nil { + r.OnChange(job.Owner, job.Repo, job.Number, job.Feature, status) + } +} + +// store writes a run's result, keyed by the inputs it was computed from, and +// returns the status it stored it under. +func store(job Job, status string, result Result, sha, digest string) string { db := config.C().DB if db == nil { - return + return status } encoded, err := result.Encode() if err != nil { @@ -317,4 +334,5 @@ func store(job Job, status string, result Result, sha, digest string) { if err := db.UpsertAIResult(job.Owner, job.Repo, job.Number, job.Feature, encoded, status, sha, digest); err != nil { slog.Error("Failed to store AI feature result", "feature", job.Feature, "repo", job.Repo, "pr", job.Number, "error", err) } + return status } diff --git a/ai/runner_test.go b/ai/runner_test.go index f0959ff2..57010254 100644 --- a/ai/runner_test.go +++ b/ai/runner_test.go @@ -5,9 +5,11 @@ import ( "crs/config" "crs/database" "errors" + "fmt" "log/slog" "os" "path/filepath" + "reflect" "strings" "sync" "sync/atomic" @@ -466,3 +468,63 @@ func waitFor(t *testing.T, cond func() bool) { time.Sleep(10 * time.Millisecond) } } + +// changeLog records what a runner's OnChange is told, in order. +type changeLog struct { + mu sync.Mutex + changes []string +} + +func (l *changeLog) record(owner, repo string, number int, feature, status string) { + l.mu.Lock() + defer l.mu.Unlock() + l.changes = append(l.changes, fmt.Sprintf("%s/%s#%d:%s %s", owner, repo, number, feature, status)) +} + +func (l *changeLog) get() []string { + l.mu.Lock() + defer l.mu.Unlock() + return append([]string(nil), l.changes...) +} + +func TestRunnerAnnouncesARunStartingAndLanding(t *testing.T) { + release := make(chan struct{}) + f := &fakeFeature{id: "fake", run: func(context.Context, Request) (Result, error) { + <-release + return Result{}, nil + }} + failing := &fakeFeature{id: "failing", run: func(context.Context, Request) (Result, error) { + return Result{}, errors.New("boom") + }} + r, _, _ := testRunner(t, nil, f, failing) + var log changeLog + r.OnChange = log.record + + r.Dispatch(job("fake", TriggerExplicit, "s", "d")) + // Pending is announced as the run is claimed, before Dispatch returns: a + // client that hears it reads the run as in flight. + if got := log.get(); !reflect.DeepEqual(got, []string{"acme/widgets#42:fake pending"}) { + t.Fatalf("after dispatch: %v", got) + } + // A run already in flight is no change. + r.Dispatch(job("fake", TriggerRerun, "s", "d")) + close(release) + waitFor(t, func() bool { return len(log.get()) == 2 }) + if r.Running("acme", "widgets", 42, "fake") { + t.Error("the landing was announced while the run still read as in flight") + } + // Up to date: nothing runs, nothing changes. + r.RunSync(job("fake", TriggerExplicit, "s", "d")) + + r.RunSync(job("failing", TriggerExplicit, "s", "d")) + + want := []string{ + "acme/widgets#42:fake pending", + "acme/widgets#42:fake success", + "acme/widgets#42:failing pending", + "acme/widgets#42:failing error", + } + if got := log.get(); !reflect.DeepEqual(got, want) { + t.Errorf("announced %v, want %v", got, want) + } +} diff --git a/bun_client/e2e/README.md b/bun_client/e2e/README.md index a2a9029e..e8281653 100644 --- a/bun_client/e2e/README.md +++ b/bun_client/e2e/README.md @@ -6,7 +6,7 @@ no GitHub token, network, or SQLite database: | Bridge spawns | E2E stand-in | What it does | | ------------------------------------------ | --------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `crs --server` (Go backend) | `harness/fake_crs.ts` | Newline-delimited JSON-RPC over stdio, serving the PRs in `fixtures/prs.ts`, with in-memory comments, reviews and feedback. Enables the comments-addressed AI feature, whose runs read `pending` for one poll; change-diagram is off unless a spec calls `backend.setDiagram`. Review-ease is on, rating three of the four PRs, unless a spec calls `backend.setReviewEase(false)`. | +| `crs --server` (Go backend) | `harness/fake_crs.ts` | Newline-delimited JSON-RPC over stdio, serving the PRs in `fixtures/prs.ts`, with in-memory comments, reviews and feedback. Enables the comments-addressed AI feature, whose runs read `pending` for a moment; change-diagram is off unless a spec calls `backend.setDiagram`. Review-ease is on, rating three of the four PRs, unless a spec calls `backend.setReviewEase(false)`. | | `diff-lsp` | `harness/fake_lsp.ts` | Reads the bridge's tempfile like the real diff-lsp and resolves positions the same way (1-indexed tempfile line, raw diff column). | | `typescript-language-server` (code viewer) | `harness/fake_lsp.ts` | Plain 0-indexed positions into the document from `didOpen`. | @@ -18,6 +18,16 @@ It serves filtered diffs too (`ListDiffFilters`, `GetFilteredDiff`): on `acme/widgets#42`, "Since my last review" is the `src/main.ts` part of its diff; every other filter and PR has no baseline. +It runs plugin re-runs and AI runs the way the server does: in the +background, on a timer, announcing each step with a `Server.PluginOutputChanged` +or `Server.AIOutputChanged` notification (a re-run is cleared, then marked +pending, then lands; an AI run is pending, then lands). The pages read the +output again on each announcement and never poll for it. `backend.holdRuns(true)` +keeps runs short of landing until `backend.holdRuns(false)`, so a spec can look +at a pending run, or check that nothing is asked for while one is in flight; +`backend.runPlugin` and `backend.runAIFeature` start a run as the server would +on its own, with nothing on the page asking for it. + Both LSP roles answer hover / definition / references / typeDefinition from the declarations in `fixtures/repo`, the checkout PR `acme/widgets#42` points at. diff --git a/bun_client/e2e/ai_report.e2e.ts b/bun_client/e2e/ai_report.e2e.ts index e7bdddba..cbb12e7e 100644 --- a/bun_client/e2e/ai_report.e2e.ts +++ b/bun_client/e2e/ai_report.e2e.ts @@ -4,7 +4,8 @@ import { expect, modal, openReview, test } from './harness/test'; // The fake backend enables comments-addressed and serves a canned report for // acme/widgets#42: bob's thread on src/greet.ts is unresolved, with alice's -// reply after the latest commit. A run reads "pending" for one poll first. +// reply after the latest commit. A run reads "pending" until it lands a moment +// later (or until a spec lifts backend.holdRuns), announcing both. const button = /Comments addressed\?/; @@ -28,15 +29,13 @@ test.describe('AI report', () => { await openReview(page); const toolbarButton = page.getByRole('button', { name: button }); await expect(toolbarButton).toBeEnabled(); - const loads = (await backend.calls('GetAIOutput')).length; - // Held, the first poll keeps the run in flight until it's released. - await backend.holdNext('GetAIOutput'); + // Held, the run stays in flight until it's released. + await backend.holdRuns(true); await toolbarButton.click(); // Never run for this PR: the click asks for a run, and the button // spins until it lands rather than opening an empty report. - await expect.poll(async () => (await backend.calls('GetAIOutput')).length).toBe(loads + 1); await expect(toolbarButton).toBeDisabled(); await expect(modal(page, 'Comments addressed?')).toHaveCount(0); const runs = await backend.calls('RunAIFeature'); @@ -50,8 +49,16 @@ test.describe('AI report', () => { }, ]); + // Nothing is asked of the server while the run is in flight: it says + // when the run lands. + await page.waitForTimeout(500); + const reads = (await backend.calls('GetAIOutput')).length; + await page.waitForTimeout(3000); + expect(await backend.calls('GetAIOutput')).toHaveLength(reads); + await expect(toolbarButton).toBeDisabled(); + // Once it lands, the toolbar says so and counts what needs attention. - await backend.release('GetAIOutput'); + await backend.holdRuns(false); await expect(page.getByRole('status')).toHaveText('✦ Comments addressed? is ready'); const counted = page.getByRole('button', { name: /Comments addressed\? \(1\)/ }); await expect(counted).toBeEnabled(); @@ -78,6 +85,18 @@ test.describe('AI report', () => { ).toBeVisible(); }); + test('counts a run the server started on its own', async ({ page, backend }) => { + await openReview(page); + await expect(page.getByRole('button', { name: button })).toBeEnabled(); + + // An automatic run: nothing on the page asked for it. + await backend.runAIFeature('comments-addressed'); + await expect( + page.getByRole('button', { name: /Comments addressed\? \(1\)/ }) + ).toBeEnabled(); + expect(await backend.calls('RunAIFeature')).toHaveLength(0); + }); + test('says so when the run cannot start', async ({ page, backend }) => { await openReview(page); const toolbarButton = page.getByRole('button', { name: button }); @@ -133,7 +152,10 @@ test.describe('AI report', () => { test.describe('AI view', () => { const aiView = '/?owner=acme&repo=widgets&number=42&view=ai'; - test('runs comments-addressed on open and polls', async ({ page, backend }) => { + test('runs comments-addressed on open and shows it once it lands', async ({ + page, + backend, + }) => { await page.goto(aiView); await expect(page).toHaveTitle('AI acme/widgets::42'); @@ -145,7 +167,8 @@ test.describe('AI view', () => { // The open thread shows the code it was left on. await expect(page.getByTestId('ai-item-code')).toContainText('+ punctuation: string;'); - // Never run for this PR, so opening the page asked for a run, then polled. + // Never run for this PR, so opening the page asked for a run, and read + // the result once the server announced it. const runs = await backend.calls('RunAIFeature'); expect(runs.map(c => c.params)).toEqual([ { diff --git a/bun_client/e2e/harness/fake_crs.ts b/bun_client/e2e/harness/fake_crs.ts index 5ca4c2ff..3f800722 100644 --- a/bun_client/e2e/harness/fake_crs.ts +++ b/bun_client/e2e/harness/fake_crs.ts @@ -50,17 +50,18 @@ interface State { // AI features: whether comments-addressed is enabled, whether review-ease // is (the fixtures' ratings are served only while it is), whether // change-diagram is and the Mermaid source it serves, the runs in flight - // (polls left before each lands) and the results there are — both keyed - // by PR and feature. + // and the results there are — both keyed by PR and feature. aiEnabled: boolean; reviewEaseEnabled: boolean; diagramEnabled: boolean; diagramSource: string; - aiRuns: Map; + aiRuns: Set; aiDone: Set; - // Plugin re-runs in flight: reads of the output left before each lands, - // keyed by PR and plugin. - pluginRuns: Map; + // Background runs (plugin re-runs, AI runs) land on a timer, as the + // server's do, unless E2E.HoldRuns holds them: then each waits here until + // the hold is lifted. + runsHeld: boolean; + heldRuns: (() => void)[]; // What the Server Configuration tab edits: UpdateConfig merges each field // it is sent into this, the way the server merges an update into the file. config: Record; @@ -117,9 +118,10 @@ function freshState(): State { reviewEaseEnabled: true, diagramEnabled: false, diagramSource: DIAGRAM_SOURCE, - aiRuns: new Map(), + aiRuns: new Set(), aiDone: new Set(), - pluginRuns: new Map(), + runsHeld: false, + heldRuns: [], config: freshConfig(), heartbeatPaused: false, }; @@ -127,6 +129,44 @@ function freshState(): State { let state = freshState(); +// Whether the bridge has subscribed to notifications: the server sends none +// before it does. +let subscribed = false; + +// How long each step of a background run takes: a plugin re-run is marked +// pending this long after it was accepted, and lands this long after that. +const RUN_STEP_MS = 300; + +/** + * Runs `step` of a background run after `ms`, unless the state was reset in + * the meantime (the next test). A step that `lands` a run waits while runs + * are held. + */ +function later(step: () => void, { lands = false, ms = RUN_STEP_MS } = {}) { + const owner = state; + setTimeout(() => { + if (state !== owner) return; + if (lands && state.runsHeld) { + state.heldRuns.push(step); + return; + } + step(); + }, ms); +} + +function prRef(pr: PRFixture) { + return { owner: pr.item.owner, repo: pr.item.repo, number: pr.item.number }; +} + +// What the server announces as a PR's plugin output or AI results change. +function pluginOutputChanged(pr: PRFixture, plugin: string, status: string) { + notify('Server.PluginOutputChanged', { ...prRef(pr), plugin, status }); +} + +function aiOutputChanged(pr: PRFixture, feature: string, status: string) { + notify('Server.AIOutputChanged', { ...prRef(pr), feature, status }); +} + // Server.Heartbeat notifications, once the bridge subscribes. They outlive // E2E.Reset: the bridge subscribes once, at startup. const STARTED_AT = new Date().toISOString(); @@ -339,8 +379,8 @@ function aiResult(pr: PRFixture, feature: string) { return commentsAddressedReport(pr); } -// One feature's output for a PR, in the shape GetAIOutput serves. A run in -// flight reads "pending" for one poll, then the canned report lands. +// One feature's output for a PR, in the shape GetAIOutput serves: "pending" +// while a run is in flight, then the canned report once it has landed. function aiOutput(pr: PRFixture, feature = AI_FEATURE) { const k = `${key(pr.item.owner, pr.item.repo, pr.item.number)}:${feature.id}`; const base = { @@ -354,21 +394,15 @@ function aiOutput(pr: PRFixture, feature = AI_FEATURE) { stale: false, truncated: false, }; - const pollsLeft = state.aiRuns.get(k); - if (pollsLeft !== undefined) { - if (pollsLeft > 0) { - state.aiRuns.set(k, pollsLeft - 1); - return { - ...base, - status: 'pending', - body: { body_type: 'markdown', body_content: '' }, - report: null, - outstanding: null, - updated_at: '', - }; - } - state.aiRuns.delete(k); - state.aiDone.add(k); + if (state.aiRuns.has(k)) { + return { + ...base, + status: 'pending', + body: { body_type: 'markdown', body_content: '' }, + report: null, + outstanding: null, + updated_at: '', + }; } if (!state.aiDone.has(k)) { return { @@ -393,37 +427,50 @@ function aiOutput(pr: PRFixture, feature = AI_FEATURE) { }; } -// A PR's plugin output, as GetPluginOutput serves it. A re-run deletes the -// plugin's result, as the server does, so the plugin is missing from the -// first read after it; it reads as pending for the next, then lands with a -// new result. -function pluginOutput(pr: PRFixture): Record { - const prefix = `${key(pr.item.owner, pr.item.repo, pr.item.number)}:`; - for (const [k, readsLeft] of state.pluginRuns) { - if (!k.startsWith(prefix)) continue; - const name = k.slice(prefix.length); - if (readsLeft === 0) { - state.pluginRuns.delete(k); - const content = `## Re-run\n\n${name} ran again.`; - pr.plugins[name] = { - result: content, - status: 'success', - body: { body_type: 'markdown', body_content: content }, - annotations: [], - }; - continue; - } - state.pluginRuns.set(k, readsLeft - 1); - if (readsLeft === 1) { - pr.plugins[name] = { - result: '', - status: 'pending', - body: { body_type: 'markdown', body_content: '' }, - annotations: [], - }; - } - } - return pr.plugins; +// Runs a plugin the way the server does, announcing each step: marked +// pending, then landing with a new result. A re-run has already deleted the +// old result, so until the first step the plugin is missing from the output. +function runPlugin(pr: PRFixture, name: string) { + later(() => { + pr.plugins[name] = { + result: '', + status: 'pending', + body: { body_type: 'markdown', body_content: '' }, + annotations: [], + }; + pluginOutputChanged(pr, name, 'pending'); + later( + () => { + const content = `## Re-run\n\n${name} ran again.`; + pr.plugins[name] = { + result: content, + status: 'success', + body: { body_type: 'markdown', body_content: content }, + annotations: [], + }; + pluginOutputChanged(pr, name, 'success'); + }, + { lands: true } + ); + }); +} + +// Starts an AI run the way the server does: pending from the claim on, +// announced before RunAIFeature replies; it lands in the background and +// announces that too. +function startAIRun(pr: PRFixture, feature: string) { + const k = `${key(pr.item.owner, pr.item.repo, pr.item.number)}:${feature}`; + state.aiDone.delete(k); + state.aiRuns.add(k); + aiOutputChanged(pr, feature, 'pending'); + later( + () => { + state.aiRuns.delete(k); + state.aiDone.add(k); + aiOutputChanged(pr, feature, 'success'); + }, + { lands: true } + ); } // Lines of a file in the fixture checkout, for GetHunkContext. @@ -609,14 +656,16 @@ const handlers: Record unknown> = { 'RPCHandler.ListPlugins': () => ({ plugins: ['summarize'] }), - 'RPCHandler.GetPluginOutput': args => ({ output: pluginOutput(findPR(args)) }), + 'RPCHandler.GetPluginOutput': args => ({ output: findPR(args).plugins }), + // Clears each result and announces it before replying, then runs the + // plugins in the background, as the server does. 'RPCHandler.RerunPlugins': args => { const pr = findPR(args); - const prefix = `${key(pr.item.owner, pr.item.repo, pr.item.number)}:`; for (const name of args.Plugins ?? []) { delete pr.plugins[name]; - state.pluginRuns.set(prefix + name, 2); + pluginOutputChanged(pr, name, ''); + runPlugin(pr, name); } return { okay: true }; }, @@ -698,8 +747,7 @@ const handlers: Record unknown> = { output: aiOutput(pr, feature), }; } - state.aiDone.delete(k); - state.aiRuns.set(k, 1); + startAIRun(pr, feature.id); return { okay: true, outcome: 'started', @@ -718,6 +766,7 @@ const handlers: Record unknown> = { }, 'RPCHandler.Subscribe': args => { + subscribed = true; heartbeatInterval = Number(args.IntervalSeconds) > 0 ? Number(args.IntervalSeconds) : 10; clearInterval(heartbeatTimer); heartbeatTimer = setInterval(() => { @@ -729,6 +778,7 @@ const handlers: Record unknown> = { }, 'RPCHandler.Unsubscribe': () => { + subscribed = false; clearInterval(heartbeatTimer); return { okay: true }; }, @@ -780,6 +830,29 @@ const handlers: Record unknown> = { return { okay: true }; }, + // Run a plugin, or an AI feature, as the server does on its own (a + // workflow cycle, a push): nothing the page did asked for it. + 'E2E.RunPlugin': args => { + runPlugin(findPR(args), args.Plugin); + return { okay: true }; + }, + + 'E2E.RunAIFeature': args => { + startAIRun(findPR(args), args.Feature); + return { okay: true }; + }, + + // Hold background runs short of landing, or land every run held. + 'E2E.HoldRuns': args => { + state.runsHeld = !!args.held; + if (!state.runsHeld) { + const held = state.heldRuns; + state.heldRuns = []; + for (const land of held) land(); + } + return { okay: true }; + }, + // Stop or resume heartbeats. 'E2E.SetHeartbeat': args => { state.heartbeatPaused = !args.beating; @@ -798,8 +871,10 @@ function respond(id: Request['id'], result: unknown, error: string | null) { process.stdout.write(`${JSON.stringify({ id, result: error ? null : result, error })}\n`); } -// A JSON-RPC notification, as the server writes one between replies. +// A JSON-RPC notification, as the server writes one between replies, once +// the bridge has subscribed. function notify(method: string, params: unknown) { + if (!subscribed) return; process.stdout.write(`${JSON.stringify({ method, params: [params], id: null })}\n`); } diff --git a/bun_client/e2e/harness/test.ts b/bun_client/e2e/harness/test.ts index 2f2b3adb..767b13f0 100644 --- a/bun_client/e2e/harness/test.ts +++ b/bun_client/e2e/harness/test.ts @@ -75,6 +75,34 @@ class Backend { return this.rpc('E2E.SetReviewEase', { enabled }); } + /** + * Hold background runs (plugin re-runs, AI runs) short of landing, or, + * with `false`, land every run held. + */ + holdRuns(held: boolean) { + return this.rpc('E2E.HoldRuns', { held }); + } + + /** Run a plugin for a PR as the server does on its own, announcing each step. */ + runPlugin(plugin: string, pr: { owner: string; repo: string; number: number } = PR42) { + return this.rpc('E2E.RunPlugin', { + Owner: pr.owner, + Repo: pr.repo, + Number: pr.number, + Plugin: plugin, + }); + } + + /** Run an AI feature for a PR as the server does on its own, announcing each step. */ + runAIFeature(feature: string, pr: { owner: string; repo: string; number: number } = PR42) { + return this.rpc('E2E.RunAIFeature', { + Owner: pr.owner, + Repo: pr.repo, + Number: pr.number, + Feature: feature, + }); + } + /** Stop or resume the backend's heartbeats. */ setHeartbeat(beating: boolean) { return this.rpc('E2E.SetHeartbeat', { beating }); diff --git a/bun_client/e2e/pr_list.e2e.ts b/bun_client/e2e/pr_list.e2e.ts index d507a2a9..a2cdd60c 100644 --- a/bun_client/e2e/pr_list.e2e.ts +++ b/bun_client/e2e/pr_list.e2e.ts @@ -279,13 +279,35 @@ test.describe('PR list', () => { await expect(page.getByText('pending', { exact: true })).toBeVisible(); await expect(page.getByRole('heading', { name: 'Summary' })).toHaveCount(0); - // Polled until it lands, with nothing reloaded by hand. + // Shown once the server announces it landed, with nothing reloaded by hand. await expect(page.getByText('summarize ran again.')).toBeVisible(); await expect(page.getByText('pending', { exact: true })).toHaveCount(0); const [rerun] = await backend.calls('RerunPlugins'); expect(rerun.params).toMatchObject({ Plugins: ['summarize'] }); }); + test('waits on the server to announce a plugin landing rather than polling', async ({ + page, + backend, + }) => { + await page.goto('/?owner=acme&repo=widgets&number=42&view=plugins'); + await expect(page.getByRole('heading', { name: 'Summary' })).toBeVisible(); + + await backend.holdRuns(true); + await page.getByRole('button', { name: 'Re-run' }).click(); + await expect(page.getByText('pending', { exact: true })).toBeVisible(); + // Past the step that marks it pending, the run sits there, and the + // page asks for nothing while it does. + await page.waitForTimeout(1000); + const reads = (await backend.calls('GetPluginOutput')).length; + await page.waitForTimeout(3000); + expect(await backend.calls('GetPluginOutput')).toHaveLength(reads); + + await backend.holdRuns(false); + await expect(page.getByText('summarize ran again.')).toBeVisible(); + await expect(page.getByText('pending', { exact: true })).toHaveCount(0); + }); + test('opens AI reports from a row', async ({ page }) => { await page.goto('/'); await page diff --git a/bun_client/e2e/review.e2e.ts b/bun_client/e2e/review.e2e.ts index ff34bcf6..c2f5dc46 100644 --- a/bun_client/e2e/review.e2e.ts +++ b/bun_client/e2e/review.e2e.ts @@ -348,13 +348,25 @@ test.describe('review view', () => { await expect(page.getByText('PENDING', { exact: true })).toBeVisible(); await expect(page.getByRole('heading', { name: 'Summary', exact: true })).toHaveCount(0); - // Polled until it lands, without closing and reopening the panel. + // Shown once the server announces it landed, without closing and + // reopening the panel. await expect(page.getByText('summarize ran again.')).toBeVisible(); await expect(page.getByText('PENDING', { exact: true })).toHaveCount(0); const [rerun] = await backend.calls('RerunPlugins'); expect(rerun.params).toMatchObject({ Plugins: ['summarize'] }); }); + test('shows a plugin run the server started on its own', async ({ page, backend }) => { + await openReview(page); + await page.getByRole('button', { name: /^Plugins/ }).click(); + await expect(page.getByRole('heading', { name: 'Summary', exact: true })).toBeVisible(); + + // A push, say: nothing on the page asked for this run. + await backend.runPlugin('summarize'); + await expect(page.getByText('summarize ran again.')).toBeVisible(); + expect(await backend.calls('RerunPlugins')).toHaveLength(0); + }); + test('starts every plugin collapsed when the output is over 300 lines', async ({ page }) => { await openReview( page, diff --git a/bun_client/frontend/src/ai_utils.test.ts b/bun_client/frontend/src/ai_utils.test.ts index 1b5261c0..913f9596 100644 --- a/bun_client/frontend/src/ai_utils.test.ts +++ b/bun_client/frontend/src/ai_utils.test.ts @@ -8,7 +8,6 @@ import { hasResult, isPending, itemLocation, - pollDelayMs, reportFeatures, reportImageName, shouldRunOnOpen, @@ -226,10 +225,6 @@ describe('presentation', () => { expect(verdictVariant('outstanding')).toBe('danger'); expect(verdictLabel('no-comments')).toBe('No comments'); }); - - test('polling backs off', () => { - expect(pollDelayMs(0)).toBeLessThan(pollDelayMs(20)); - }); }); describe('change-diagram', () => { diff --git a/bun_client/frontend/src/ai_utils.ts b/bun_client/frontend/src/ai_utils.ts index 0a15baf9..78a80f33 100644 --- a/bun_client/frontend/src/ai_utils.ts +++ b/bun_client/frontend/src/ai_utils.ts @@ -271,14 +271,6 @@ export function shouldRunOnOpen(output: AIFeatureOutput | null | undefined): boo return output.status === 'not-run' || output.stale; } -/** How long to wait before the next poll of a pending run. */ -export function pollDelayMs(attempt: number): number { - return attempt < 10 ? 1500 : 3000; -} - -/** Give up polling a run after this long; the server's own timeout is five minutes. */ -export const MAX_POLL_MS = 6 * 60 * 1000; - export function statusVariant(status: string): StatusVariant { switch (status) { case 'success': diff --git a/bun_client/frontend/src/api.ts b/bun_client/frontend/src/api.ts index 835dfbe1..83a80a85 100644 --- a/bun_client/frontend/src/api.ts +++ b/bun_client/frontend/src/api.ts @@ -122,7 +122,8 @@ export async function listAIFeatures(): Promise { /** * Asks for an AI feature to run for a PR. The run happens in the background: - * poll getAIOutput while the output reads `pending`. Unless `Force` is set, a + * the server announces it landing (Server.AIOutputChanged), and getAIOutput + * then reads the result. Unless `Force` is set, a * stored result that still covers the PR's inputs answers without a run. */ export async function runAIFeature( diff --git a/bun_client/frontend/src/components/AIOutput.tsx b/bun_client/frontend/src/components/AIOutput.tsx index 711e9b55..485e2081 100644 --- a/bun_client/frontend/src/components/AIOutput.tsx +++ b/bun_client/frontend/src/components/AIOutput.tsx @@ -27,8 +27,8 @@ const errorText = (e: unknown) => (e instanceof Error ? e.message : String(e)); * * Each feature's card behaves like the review view's report modal: opening * the page runs a feature that never ran for the PR or went stale (the server - * answers from its cache when nothing changed), the card polls while the run - * is pending, and ↻ Re-run forces a fresh one. There is no diff here to jump + * answers from its cache when nothing changed), the card shows the result as + * soon as the server says the run landed, and ↻ Re-run forces a fresh one. There is no diff here to jump * into, so item locations are plain text; Open review goes to the PR. */ export default function AIOutput({ owner, repo, number, onOpenReview, onClose }: AIOutputProps) { @@ -225,7 +225,6 @@ export function AIFeatureCard({ output={report.output} error={report.error} notice={report.notice} - gaveUp={report.gaveUp} imageName={reportImageName(owner, repo, number, feature.id)} /> diff --git a/bun_client/frontend/src/components/AIReportModal.tsx b/bun_client/frontend/src/components/AIReportModal.tsx index 18aad25f..35153d32 100644 --- a/bun_client/frontend/src/components/AIReportModal.tsx +++ b/bun_client/frontend/src/components/AIReportModal.tsx @@ -27,8 +27,8 @@ interface AIReportModalProps { * One AI feature's result for the PR under review. * * Opening it asks the server for a run when there is no result yet or the - * stored one no longer describes the PR, then polls while the run is pending - * (see useAIReport). The previous result stays on screen, marked as + * stored one no longer describes the PR, then shows the new result as soon as + * the server says it landed (see useAIReport). The previous result stays on screen, marked as * refreshing, until the new one lands. * * The change diagram takes most of the screen, so a large diagram stays @@ -74,7 +74,6 @@ export default function AIReportModal({ output={report.output} error={report.error} notice={report.notice} - gaveUp={report.gaveUp} onJumpToItem={onJumpToItem} fill={diagram} imageName={reportImageName(owner, repo, number, feature.id)} diff --git a/bun_client/frontend/src/components/AIReportView.tsx b/bun_client/frontend/src/components/AIReportView.tsx index 24a1e806..c5b7f384 100644 --- a/bun_client/frontend/src/components/AIReportView.tsx +++ b/bun_client/frontend/src/components/AIReportView.tsx @@ -26,7 +26,6 @@ interface AIReportViewProps { output: AIFeatureOutput | null; error?: string | null; notice?: string | null; - gaveUp?: boolean; /** Takes the reviewer to an item's thread in the diff. Without it, locations are plain text. */ onJumpToItem?: (item: ReportItem) => void; /** Grow to fill the parent, a flex column, as the change diagram does in its modal. */ @@ -47,7 +46,6 @@ export default function AIReportView({ output, error, notice, - gaveUp, onJumpToItem, fill, imageName, @@ -67,11 +65,6 @@ export default function AIReportView({ {error && Could not load the report: {error}} {notice && {notice}} - {gaveUp && ( - - Still running after several minutes — close this and check back later. - - )} {output?.stale && !pending && ( The PR has changed since this report was made. Re-run it for a current answer. diff --git a/bun_client/frontend/src/components/Review.tsx b/bun_client/frontend/src/components/Review.tsx index 0c564c6c..c76aa037 100644 --- a/bun_client/frontend/src/components/Review.tsx +++ b/bun_client/frontend/src/components/Review.tsx @@ -116,7 +116,8 @@ export default function Review({ const [outdatedComments, setOutdatedComments] = useState([]); const [reviews, setReviews] = useState([]); const [metadata, setMetadata] = useState(null); - // Loaded when the PR opens, and polled while any plugin is running. + // Loaded when the PR opens, and read again as the server announces plugins + // starting and finishing. const { outputs: pluginOutputs, executing: executingPlugins, diff --git a/bun_client/frontend/src/hooks/useAIBackgroundRuns.ts b/bun_client/frontend/src/hooks/useAIBackgroundRuns.ts index 4c19281d..2f477b90 100644 --- a/bun_client/frontend/src/hooks/useAIBackgroundRuns.ts +++ b/bun_client/frontend/src/hooks/useAIBackgroundRuns.ts @@ -1,13 +1,15 @@ import { useEffect, useRef, useState } from 'react'; import { getAIOutput, rpcErrorMessage, runAIFeature } from '../api'; -import { - hasResult, - isPending, - pollDelayMs, - type AIFeatureInfo, - type AIFeatureOutput, -} from '../ai_utils'; +import { hasResult, isPending, type AIFeatureInfo, type AIFeatureOutput } from '../ai_utils'; import type { StatusVariant } from '../design'; +import { + AI_OUTPUT_CHANGED_EVENT, + isChangeFor, + parseAIOutputChanged, + serverEvents, + type ServerEvents, +} from '../server_events'; +import { useServerChanges } from './useServerChanges'; interface UseAIBackgroundRunsOptions { owner: string; @@ -18,9 +20,10 @@ interface UseAIBackgroundRunsOptions { outputs: Record; /** Merges the outputs this hook loads into the caller's. */ onOutputs: (outputs: Record) => void; - /** The feature whose report is open; its modal polls for itself. */ + /** The feature whose report is open; its modal follows it for itself. */ openFeature: string | null; onToast: (message: string, variant?: StatusVariant) => void; + events?: ServerEvents; } export interface AIBackgroundRuns { @@ -35,9 +38,10 @@ export interface AIBackgroundRuns { * * A feature with no result for the PR has no report worth opening, so its * button asks for a run instead (`generate`) and spins until it lands, then - * says so with a toast. Any feature whose run is in flight — asked for here, - * or started automatically by the server — is polled, so the toolbar's - * spinners and counts stay current without a report open. + * says so with a toast. The server announces every run of a feature for the + * PR as it starts and lands — asked for here, or started by the server itself + * — and the outputs are read again on each, so the toolbar's spinners and + * counts stay current without a report open, and without polling. */ export function useAIBackgroundRuns({ owner, @@ -48,6 +52,7 @@ export function useAIBackgroundRuns({ onOutputs, openFeature, onToast, + events = serverEvents, }: UseAIBackgroundRunsOptions): AIBackgroundRuns { const pr = `${owner}/${repo}#${number}`; // Features whose RunAIFeature request is in flight, before its reply @@ -59,7 +64,7 @@ export function useAIBackgroundRuns({ const prRef = useRef(pr); // Read through refs so a parent re-render (new callback identities) - // doesn't restart the polling. + // doesn't resubscribe. const onOutputsRef = useRef(onOutputs); const onToastRef = useRef(onToast); useEffect(() => { @@ -73,35 +78,27 @@ export function useAIBackgroundRuns({ setStarting(new Set()); }, [pr]); - // The features to poll for: every run in flight but the open report's. - const polling = features - .filter(f => f.id !== openFeature && isPending(outputs[f.id])) - .map(f => f.id) - .join(','); - - useEffect(() => { - if (!polling) return; - let cancelled = false; - let timer: ReturnType | undefined; - const poll = async (attempt: number) => { - try { - const loaded = await getAIOutput({ Owner: owner, Repo: repo, Number: number }); - if (cancelled) return; - onOutputsRef.current(loaded); - } catch (e) { - if (cancelled) return; - console.error('Failed to poll AI outputs:', e); - } - // Stops once nothing is pending: `polling` empties and this - // effect is cleaned up. - timer = setTimeout(() => poll(attempt + 1), pollDelayMs(attempt)); - }; - timer = setTimeout(() => poll(0), pollDelayMs(0)); - return () => { - cancelled = true; - if (timer) clearTimeout(timer); - }; - }, [owner, repo, number, polling]); + // Read every output again when the server says one of the toolbar's + // features changed for this PR. The open report's modal reads its own and + // passes it on. + useServerChanges( + AI_OUTPUT_CHANGED_EVENT, + data => { + const change = parseAIOutputChanged(data); + return ( + !!change && + change.feature !== openFeature && + features.some(f => f.id === change.feature) && + isChangeFor(change, { owner, repo, number }) + ); + }, + async () => { + const startedFor = prRef.current; + const loaded = await getAIOutput({ Owner: owner, Repo: repo, Number: number }); + if (prRef.current === startedFor) onOutputsRef.current(loaded); + }, + { key: pr, enabled: features.length > 0, events } + ); useEffect(() => { for (const id of requested.current) { diff --git a/bun_client/frontend/src/hooks/useAIReport.ts b/bun_client/frontend/src/hooks/useAIReport.ts index 44bd5e2d..00099284 100644 --- a/bun_client/frontend/src/hooks/useAIReport.ts +++ b/bun_client/frontend/src/hooks/useAIReport.ts @@ -1,12 +1,14 @@ import { useEffect, useRef, useState } from 'react'; import { getAIOutput, runAIFeature } from '../api'; +import { isPending, shouldRunOnOpen, type AIFeatureOutput } from '../ai_utils'; import { - isPending, - MAX_POLL_MS, - pollDelayMs, - shouldRunOnOpen, - type AIFeatureOutput, -} from '../ai_utils'; + AI_OUTPUT_CHANGED_EVENT, + isChangeFor, + parseAIOutputChanged, + serverEvents, + type ServerEvents, +} from '../server_events'; +import { useServerChanges } from './useServerChanges'; interface UseAIReportOptions { owner: string; @@ -17,6 +19,7 @@ interface UseAIReportOptions { initialOutput?: AIFeatureOutput; /** Every output loaded, so a count shown elsewhere stays current. */ onOutput?: (output: AIFeatureOutput) => void; + events?: ServerEvents; } export interface AIReportState { @@ -25,8 +28,6 @@ export interface AIReportState { error: string | null; /** Why the server declined to run the feature. */ notice: string | null; - /** Polling stopped with the run still pending. */ - gaveUp: boolean; pending: boolean; /** Runs the feature again, even if nothing changed. */ rerun: () => void; @@ -39,8 +40,10 @@ const errorText = (e: unknown) => (e instanceof Error ? e.message : String(e)); * * Mounting asks the server for a run when there is no result yet or the * stored one no longer describes the PR (the server answers from its cache - * when nothing changed), then polls while the run is pending. The previous - * result stays in `output` until the new one lands. + * when nothing changed). The server announces each run of the feature for + * the PR as it starts and as it lands, and the output is read again on each, + * so a pending run shows its result without polling. The previous result + * stays in `output` until the new one lands. */ export function useAIReport({ owner, @@ -49,13 +52,14 @@ export function useAIReport({ feature, initialOutput, onOutput, + events = serverEvents, }: UseAIReportOptions): AIReportState { const [output, setOutput] = useState(initialOutput ?? null); const [error, setError] = useState(null); const [notice, setNotice] = useState(null); - const [gaveUp, setGaveUp] = useState(false); // Bumped by rerun(); each value is one forced run. const [rerunToken, setRerunToken] = useState(0); + const key = `${owner}/${repo}#${number}:${feature}`; // Read through a ref so a parent re-render (a new callback identity) // doesn't restart the run. @@ -64,56 +68,45 @@ export function useAIReport({ onOutputRef.current = onOutput; }, [onOutput]); - useEffect(() => { - let cancelled = false; - let timer: ReturnType | undefined; - const pr = { Owner: owner, Repo: repo, Number: number, Feature: feature }; - const startedAt = Date.now(); + // Replies can arrive out of order — a run's reply, written before the + // run landed, after the read its landing prompted — so each request takes + // a ticket and only a reply newer than the last one shown is shown. The + // key guards against a reply for the feature or PR just switched away + // from. + const lastTicket = useRef(0); + const lastShown = useRef(0); + const keyRef = useRef(key); + const ticket = () => ({ seq: ++lastTicket.current, key }); + const show = (t: { seq: number; key: string }, o: AIFeatureOutput | undefined) => { + if (!o || t.key !== keyRef.current || t.seq <= lastShown.current) return; + lastShown.current = t.seq; + setOutput(o); + onOutputRef.current?.(o); + }; - const publish = (o: AIFeatureOutput) => { - setOutput(o); - onOutputRef.current?.(o); - }; + const args = { Owner: owner, Repo: repo, Number: number, Feature: feature }; - const poll = async (attempt: number) => { - try { - const o = (await getAIOutput(pr))[feature]; - if (cancelled || !o) return; - publish(o); - if (!isPending(o)) return; - if (Date.now() - startedAt > MAX_POLL_MS) { - setGaveUp(true); - return; - } - timer = setTimeout(() => poll(attempt + 1), pollDelayMs(attempt)); - } catch (e) { - if (!cancelled) setError(errorText(e)); - } - }; + useEffect(() => { + keyRef.current = key; + let cancelled = false; const start = async () => { setError(null); setNotice(null); - setGaveUp(false); try { let current: AIFeatureOutput | undefined; if (rerunToken === 0) { - current = (await getAIOutput(pr))[feature]; + const t = ticket(); + current = (await getAIOutput(args))[feature]; if (cancelled) return; - if (current) publish(current); + show(t, current); } if (rerunToken > 0 || shouldRunOnOpen(current)) { - const reply = await runAIFeature({ ...pr, Force: rerunToken > 0 }); + const t = ticket(); + const reply = await runAIFeature({ ...args, Force: rerunToken > 0 }); if (cancelled) return; - if (reply.output) publish(reply.output); - if (!reply.okay) { - setNotice(reply.message); - return; - } - current = reply.output ?? current; - } - if (current && isPending(current)) { - timer = setTimeout(() => poll(0), pollDelayMs(0)); + show(t, reply.output ?? undefined); + if (!reply.okay) setNotice(reply.message); } } catch (e) { if (!cancelled) setError(errorText(e)); @@ -123,15 +116,36 @@ export function useAIReport({ start(); return () => { cancelled = true; - if (timer) clearTimeout(timer); }; }, [owner, repo, number, feature, rerunToken]); + // Read again whenever the server says this feature's output for the PR + // changed: a run starting or landing, whoever asked for it. + useServerChanges( + AI_OUTPUT_CHANGED_EVENT, + data => { + const change = parseAIOutputChanged(data); + return ( + !!change && + change.feature === feature && + isChangeFor(change, { owner, repo, number }) + ); + }, + async () => { + const t = ticket(); + try { + show(t, (await getAIOutput(args))[feature]); + } catch (e) { + if (t.key === keyRef.current) setError(errorText(e)); + } + }, + { key, events } + ); + return { output, error, notice, - gaveUp, pending: isPending(output), rerun: () => setRerunToken(t => t + 1), }; diff --git a/bun_client/frontend/src/hooks/usePluginOutputs.ts b/bun_client/frontend/src/hooks/usePluginOutputs.ts index 57fe7b93..d66f945e 100644 --- a/bun_client/frontend/src/hooks/usePluginOutputs.ts +++ b/bun_client/frontend/src/hooks/usePluginOutputs.ts @@ -1,18 +1,24 @@ import { useEffect, useMemo, useRef, useState } from 'react'; import { rpcCall } from '../api'; import { - MAX_PLUGIN_POLL_MS, - PLUGIN_POLL_MS, - pendingPluginsKey, + MAX_RERUN_WAIT_MS, rerunsStillAwaited, withAwaitedReruns, type PluginResult, } from '../plugin_utils'; +import { + isChangeFor, + parsePluginOutputChanged, + PLUGIN_OUTPUT_CHANGED_EVENT, + serverEvents, + type ServerEvents, +} from '../server_events'; +import { useServerChanges } from './useServerChanges'; export interface PluginOutputs { /** Each plugin's latest output; a re-run whose result is yet to land reads as pending. */ outputs: Record; - /** Whether a load someone asked for is in flight. Polls don't count. */ + /** Whether a load someone asked for is in flight. Re-reads the server asked for don't count. */ loading: boolean; /** Plugins whose re-run request is in flight. */ executing: Set; @@ -26,12 +32,17 @@ export interface PluginOutputs { * A PR's plugin output, kept current while plugins run. * * The server runs a plugin in the background, so neither a re-run nor the - * first automatic run has a result to hand back right away. While any plugin - * is pending the output is read again every two seconds, one read at a time, - * until each has landed — or until a plugin has been pending for longer than - * the server lets one run. + * first automatic run has a result to hand back right away. Instead the + * server announces every change to a PR's plugin output — a plugin marked + * pending, a result landing, a re-run clearing one — and the output is read + * again when an announcement names this PR. Nothing is polled. */ -export function usePluginOutputs(owner: string, repo: string, number: number): PluginOutputs { +export function usePluginOutputs( + owner: string, + repo: string, + number: number, + events: ServerEvents = serverEvents +): PluginOutputs { const pr = `${owner}/${repo}#${number}`; // The output as the server last served it, and the re-runs it doesn't // show the result of yet. @@ -39,7 +50,7 @@ export function usePluginOutputs(owner: string, repo: string, number: number): P const [awaiting, setAwaiting] = useState>(new Set()); const [executing, setExecuting] = useState>(new Set()); const [loading, setLoading] = useState(false); - // Only the latest read's reply is applied, so a poll sent before a re-run + // Only the latest read's reply is applied, so a read sent before a re-run // was accepted can't report the result the re-run replaced, and a reply // for the PR just navigated away from can't paint onto this one. const latestRead = useRef(0); @@ -76,35 +87,27 @@ export function usePluginOutputs(owner: string, repo: string, number: number): P read(false); }, [pr]); + // Read again whenever the server says this PR's output changed. + useServerChanges( + PLUGIN_OUTPUT_CHANGED_EVENT, + data => { + const change = parsePluginOutputChanged(data); + return !!change && isChangeFor(change, { owner, repo, number }); + }, + () => read(true), + { key: pr, events } + ); + const outputs = useMemo(() => withAwaitedReruns(stored, awaiting), [stored, awaiting]); - const polling = pendingPluginsKey(outputs); - // Restarts whenever the set of pending plugins changes, so the time limit - // runs from the last plugin to start or finish. + // A re-run that never lands (a server restart lost it, or it never + // started) is announced by nothing, so it stops reading as pending once + // the server would have stopped it. Restarts as re-runs start or land. useEffect(() => { - if (!polling) return; - const startedAt = Date.now(); - let cancelled = false; - let timer: ReturnType | undefined; - const poll = async () => { - if (Date.now() - startedAt > MAX_PLUGIN_POLL_MS) { - // Pending for longer than the server lets a plugin run: the - // run was lost (a server restart) or never started. Stop - // standing in for a result that isn't coming. - setAwaiting(new Set()); - return; - } - await read(true); - // Stops once nothing is pending: `polling` empties and this effect - // is cleaned up. - if (!cancelled) timer = setTimeout(poll, PLUGIN_POLL_MS); - }; - timer = setTimeout(poll, PLUGIN_POLL_MS); - return () => { - cancelled = true; - if (timer) clearTimeout(timer); - }; - }, [pr, polling]); + if (awaiting.size === 0) return; + const timer = setTimeout(() => setAwaiting(new Set()), MAX_RERUN_WAIT_MS); + return () => clearTimeout(timer); + }, [awaiting]); const execute = async (name: string) => { const startedFor = prRef.current; @@ -115,7 +118,7 @@ export function usePluginOutputs(owner: string, repo: string, number: number): P ]); if (prRef.current !== startedFor) return; // The server has cleared the old result; the plugin reads as - // pending, and is polled for, until the new one lands. + // pending until the server announces the new one. setAwaiting(prev => new Set(prev).add(name)); await read(true); } catch (e) { diff --git a/bun_client/frontend/src/hooks/useServerChanges.ts b/bun_client/frontend/src/hooks/useServerChanges.ts new file mode 100644 index 00000000..19c425ff --- /dev/null +++ b/bun_client/frontend/src/hooks/useServerChanges.ts @@ -0,0 +1,72 @@ +import { useEffect, useRef } from 'react'; +import { serverEvents, type ServerEvents } from '../server_events'; + +export interface ServerChangesOptions { + /** Resubscribes when it changes: the PR (and feature) being shown. */ + key: string; + /** Listen only while this is set. */ + enabled?: boolean; + events?: ServerEvents; +} + +/** + * Calls `refresh` whenever the server pushes an `event` that `matches` + * accepts, and whenever the event stream (re)connects: anything sent while it + * was down never arrives, so that is when to catch up. This is how a view + * keeps output the server produces in the background current without polling. + * + * One refresh runs at a time. Changes come in bursts (a run announces itself + * starting, then landing), so any that arrive while a refresh is in flight ask + * for one more once it's done rather than one each. + * + * `matches` and `refresh` are read when needed, so new identities on each + * render don't resubscribe; a new `key` does. + */ +export function useServerChanges( + event: string, + matches: (data: unknown) => boolean, + refresh: () => Promise, + { key, enabled = true, events = serverEvents }: ServerChangesOptions +): void { + const matchesRef = useRef(matches); + const refreshRef = useRef(refresh); + useEffect(() => { + matchesRef.current = matches; + refreshRef.current = refresh; + }); + + useEffect(() => { + if (!enabled) return; + let active = true; + let running = false; + let again = false; + const run = async () => { + if (running) { + again = true; + return; + } + running = true; + try { + do { + again = false; + try { + await refreshRef.current(); + } catch (e) { + console.error(`Failed to refresh after ${event}:`, e); + } + } while (again && active); + } finally { + running = false; + } + }; + const offEvent = events.on(event, data => { + if (matchesRef.current(data)) void run(); + }); + const offOpen = events.onOpen(() => void run()); + return () => { + active = false; + offEvent(); + offOpen(); + }; + }, [event, key, enabled, events]); +} diff --git a/bun_client/frontend/src/plugin_utils.test.ts b/bun_client/frontend/src/plugin_utils.test.ts index c049e773..8c5c54d8 100644 --- a/bun_client/frontend/src/plugin_utils.test.ts +++ b/bun_client/frontend/src/plugin_utils.test.ts @@ -5,7 +5,6 @@ import { collapsePluginsByDefault, COLLAPSE_PLUGINS_OVER_LINES, isPluginPending, - pendingPluginsKey, pluginOutputLineCount, rerunsStillAwaited, resolvePluginBody, @@ -205,23 +204,6 @@ describe('rerunsStillAwaited', () => { }); }); -describe('pendingPluginsKey', () => { - test('names every pending plugin, in a stable order', () => { - expect( - pendingPluginsKey({ - summarize: { result: '', status: 'pending' }, - done: { result: 'x', status: 'success' }, - lint: { result: '', status: 'pending' }, - }) - ).toBe('lint,summarize'); - }); - - test('is empty when nothing is pending', () => { - expect(pendingPluginsKey({ done: { result: 'x', status: 'success' } })).toBe(''); - expect(pendingPluginsKey({})).toBe(''); - }); -}); - describe('totalAnnotationCount', () => { test('sums annotations across plugins and ignores plugins without any', () => { const plugins: Record = { diff --git a/bun_client/frontend/src/plugin_utils.ts b/bun_client/frontend/src/plugin_utils.ts index 3faff29d..de5471fe 100644 --- a/bun_client/frontend/src/plugin_utils.ts +++ b/bun_client/frontend/src/plugin_utils.ts @@ -114,11 +114,12 @@ export function isPluginPending(plugin: PluginResult | undefined): boolean { return (plugin?.status || '').toLowerCase() === 'pending'; } -/** How often a plugin output surface reads the output again while a plugin is pending. */ -export const PLUGIN_POLL_MS = 2000; - -/** Give up polling after this long; the server stops a plugin after five minutes. */ -export const MAX_PLUGIN_POLL_MS = 6 * 60 * 1000; +/** + * Stop standing in for a re-run's result after this long without one: the + * server stops a plugin after five minutes, so a re-run that hasn't landed by + * then was lost (a server restart) or never started. + */ +export const MAX_RERUN_WAIT_MS = 6 * 60 * 1000; /** * The output to show while re-runs are awaited. A re-run deletes the plugin's @@ -151,14 +152,6 @@ export function rerunsStillAwaited( ); } -/** The plugins pending in `outputs`, as a stable key: empty when there are none. */ -export function pendingPluginsKey(outputs: Record): string { - return Object.keys(outputs) - .filter(name => isPluginPending(outputs[name])) - .sort() - .join(','); -} - /** Total number of annotations across every plugin, for summary counts. */ export function totalAnnotationCount(plugins: Record): number { return Object.values(plugins).reduce((sum, p) => sum + (p.annotations?.length ?? 0), 0); diff --git a/bun_client/frontend/src/server_events.test.tsx b/bun_client/frontend/src/server_events.test.tsx index f76e93e7..9735478e 100644 --- a/bun_client/frontend/src/server_events.test.tsx +++ b/bun_client/frontend/src/server_events.test.tsx @@ -6,7 +6,11 @@ import { heartbeatDeadlineMs, heartbeatReducer, INITIAL_HEARTBEAT_STATE, + isChangeFor, + parseAIOutputChanged, parseHeartbeat, + parsePluginOutputChanged, + PLUGIN_OUTPUT_CHANGED_EVENT, ServerEvents, type EventStream, type Heartbeat, @@ -22,6 +26,7 @@ const BEAT: Heartbeat = { /** Stands in for EventSource: records listeners, delivers events on demand. */ class FakeStream implements EventStream { listeners = new Map) => void>>(); + onopen: ((event: Event) => void) | null = null; onerror: ((event: Event) => void) | null = null; closed = false; @@ -47,6 +52,10 @@ class FakeStream implements EventStream { fail() { this.onerror?.(new Event('error')); } + + connect() { + this.onopen?.(new Event('open')); + } } function hub() { @@ -72,6 +81,42 @@ describe('parseHeartbeat', () => { }); }); +describe('change events', () => { + const pr = { owner: 'acme', repo: 'widgets', number: 42 }; + + test('reads a plugin output change', () => { + expect(parsePluginOutputChanged({ ...pr, plugin: 'summarize', status: 'success' })).toEqual( + { ...pr, plugin: 'summarize', status: 'success' } + ); + // A re-run of every plugin clears them all at once: no plugin, no status. + expect(parsePluginOutputChanged({ ...pr, plugin: '', status: '' })).toEqual({ + ...pr, + plugin: '', + status: '', + }); + }); + + test('reads an AI output change', () => { + expect( + parseAIOutputChanged({ ...pr, feature: 'comments-addressed', status: 'pending' }) + ).toEqual({ ...pr, feature: 'comments-addressed', status: 'pending' }); + expect(parseAIOutputChanged({ ...pr, status: 'pending' })).toBeNull(); + }); + + test('rejects anything that names no PR', () => { + expect(parsePluginOutputChanged(null)).toBeNull(); + expect(parsePluginOutputChanged({ ...pr, number: '42' })).toBeNull(); + expect(parseAIOutputChanged({ owner: 'acme', number: 42, feature: 'x' })).toBeNull(); + }); + + test('matches its PR whatever the case of its names', () => { + expect(isChangeFor({ owner: 'Acme', repo: 'Widgets', number: 42 }, pr)).toBe(true); + expect(isChangeFor({ ...pr, number: 43 }, pr)).toBe(false); + expect(isChangeFor({ ...pr, repo: 'gadgets' }, pr)).toBe(false); + expect(isChangeFor({ ...pr, owner: 'other' }, pr)).toBe(false); + }); +}); + describe('heartbeatReducer', () => { test('goes live on a heartbeat, lost when one is missed, and back', () => { const live = heartbeatReducer(INITIAL_HEARTBEAT_STATE, { type: 'beat', heartbeat: BEAT }); @@ -167,6 +212,21 @@ describe('ServerEvents', () => { expect(errors).toBe(1); }); + test('tells open listeners each time the stream connects', () => { + const { events, opened } = hub(); + let opens = 0; + const off = events.onOpen(() => opens++); + events.on(PLUGIN_OUTPUT_CHANGED_EVENT, () => {}); + opened[0].connect(); + // EventSource reconnects by itself after a drop, and says so again. + opened[0].fail(); + opened[0].connect(); + expect(opens).toBe(2); + off(); + opened[0].connect(); + expect(opens).toBe(2); + }); + test('does nothing where there is no EventSource', () => { const events = new ServerEvents(() => null); const off = events.on(HEARTBEAT_EVENT, () => {}); diff --git a/bun_client/frontend/src/server_events.ts b/bun_client/frontend/src/server_events.ts index 4405f053..b00769f4 100644 --- a/bun_client/frontend/src/server_events.ts +++ b/bun_client/frontend/src/server_events.ts @@ -35,6 +35,81 @@ export function parseHeartbeat(data: unknown): Heartbeat | null { return { seq, time, started_at, interval_seconds }; } +/** + * Sent whenever a PR's stored plugin output changes: a plugin marked pending, + * a result landing, a re-run clearing one. It names what changed, not the + * output; a page showing the PR reads GetPluginOutput again. + */ +export const PLUGIN_OUTPUT_CHANGED_EVENT = 'Server.PluginOutputChanged'; + +export interface PluginOutputChanged { + owner: string; + repo: string; + number: number; + /** The plugin whose result changed; empty when a re-run cleared them all. */ + plugin: string; + /** Its status now (`pending`, `success`, ...); empty when it was cleared for a re-run. */ + status: string; +} + +/** + * Sent when an AI feature's output for a PR changes: as a run starts + * (`pending`) and as its result is stored. Like a plugin change it names what + * changed; a page showing the feature reads GetAIOutput again. + */ +export const AI_OUTPUT_CHANGED_EVENT = 'Server.AIOutputChanged'; + +export interface AIOutputChanged { + owner: string; + repo: string; + number: number; + feature: string; + /** `pending` as a run starts, then the stored result's status. */ + status: string; +} + +/** A pull request, as the server's change events name one. */ +export interface PRRef { + owner: string; + repo: string; + number: number; +} + +function parsePRRef(data: unknown): (PRRef & Record) | null { + if (typeof data !== 'object' || data === null) return null; + const fields = data as Record; + const { owner, repo, number } = fields; + if (typeof owner !== 'string' || typeof repo !== 'string' || typeof number !== 'number') { + return null; + } + return { ...fields, owner, repo, number }; +} + +const text = (value: unknown) => (typeof value === 'string' ? value : ''); + +export function parsePluginOutputChanged(data: unknown): PluginOutputChanged | null { + const pr = parsePRRef(data); + if (!pr) return null; + const { owner, repo, number } = pr; + return { owner, repo, number, plugin: text(pr.plugin), status: text(pr.status) }; +} + +export function parseAIOutputChanged(data: unknown): AIOutputChanged | null { + const pr = parsePRRef(data); + if (!pr || typeof pr.feature !== 'string') return null; + const { owner, repo, number } = pr; + return { owner, repo, number, feature: pr.feature, status: text(pr.status) }; +} + +/** Whether `change` is about `pr`. GitHub names ignore case. */ +export function isChangeFor(change: PRRef, pr: PRRef): boolean { + return ( + change.number === pr.number && + change.owner.toLowerCase() === pr.owner.toLowerCase() && + change.repo.toLowerCase() === pr.repo.toLowerCase() + ); +} + /** Heartbeats that can go missing before the server is taken for gone. */ export const MISSED_HEARTBEATS = 3; @@ -81,6 +156,7 @@ type MessageListener = (event: MessageEvent) => void; export interface EventStream { addEventListener(type: string, listener: MessageListener): void; removeEventListener(type: string, listener: MessageListener): void; + onopen: ((event: Event) => void) | null; onerror: ((event: Event) => void) | null; close(): void; } @@ -95,6 +171,7 @@ export class ServerEvents { private source: EventStream | null = null; private readonly listeners = new Map>(); private readonly dispatchers = new Map(); + private readonly openListeners = new Set<() => void>(); private readonly errorListeners = new Set<() => void>(); constructor(private readonly open: () => EventStream | null) {} @@ -118,6 +195,16 @@ export class ServerEvents { }; } + /** + * Calls `listener` each time the stream connects: at first, and again + * whenever it comes back after dropping. Events sent while it was down are + * never delivered, so this is when to catch up. Returns the unsubscribe. + */ + onOpen(listener: () => void): () => void { + this.openListeners.add(listener); + return () => this.openListeners.delete(listener); + } + /** Calls `listener` whenever the stream drops. Returns the unsubscribe. */ onError(listener: () => void): () => void { this.errorListeners.add(listener); @@ -129,6 +216,9 @@ export class ServerEvents { const source = this.open(); if (!source) return; this.source = source; + source.onopen = () => { + for (const listener of [...this.openListeners]) listener(); + }; source.onerror = () => { for (const listener of [...this.errorListeners]) listener(); }; diff --git a/docs/ai_features.md b/docs/ai_features.md index 2c667a32..d90e50e6 100644 --- a/docs/ai_features.md +++ b/docs/ai_features.md @@ -215,7 +215,10 @@ change-diagram, file-ordering and review-ease run `oneshot` only. ## Running - **On demand.** A client asks with `RunAIFeature`; the run happens in the - background and the client polls `GetAIOutput` while it reads `pending`. + background. The server announces every run, on demand or automatic, as it + starts and as it lands ([`Server.AIOutputChanged`](protocol.md#serveraioutputchanged)), + and a client showing the feature reads `GetAIOutput` again on each instead of + polling. Opening a report in the web or Emacs client asks for a run when the feature has never run for the PR or its report is [stale](#caching-and-staleness); the Chrome extension leaves that to its **Run** button. The web diff --git a/docs/protocol.md b/docs/protocol.md index ff637938..046d90f1 100644 --- a/docs/protocol.md +++ b/docs/protocol.md @@ -46,6 +46,42 @@ Sent every interval while a client is subscribed. If a few intervals pass withou | `started_at` | string | When the server started (RFC 3339); a new value means a new server process | | `interval_seconds` | int | How long until the next heartbeat | +### `Server.PluginOutputChanged` + +Sent each time a plugin's stored result for a PR is written or cleared, so what [`GetPluginOutput`](#rpchandlergetpluginoutput) returns for that PR has changed: a plugin is marked `pending` as its run starts, then `success` or `error` as it ends; an on-demand plugin is recorded `deferred` whenever the PR's plugins run; and [`RerunPlugins`](#rpchandlerrerunplugins) clears the results it reruns before it replies. A client showing a PR's plugin output reads `GetPluginOutput` again when one names that PR, instead of polling it. The notification names what changed, never the output itself, which can be large. + +```json +{"method": "Server.PluginOutputChanged", "params": [{"owner": "C-Hipple", "repo": "code-review-server", "number": 303, "plugin": "summarize", "status": "success"}], "id": null} +``` + +| Field | Type | Description | +|----------|--------|---------------------------------------------------------------------------------------------------| +| `owner` | string | Repository owner, as the run was asked for it | +| `repo` | string | Repository name (the short name, as everywhere else) | +| `number` | int | Pull request number | +| `plugin` | string | The plugin whose result changed; empty when `RerunPlugins` cleared every plugin's result at once | +| `status` | string | The plugin's status as `GetPluginOutput` now reports it (`pending`, `success`, `error`, `deferred`), or empty when its result was cleared for a rerun: the plugin is missing from the output until the run marks it `pending` | + +Only a subscribed client hears these, and only while it is subscribed: a client that was not (or whose connection dropped) reads `GetPluginOutput` once on (re)subscribing to catch up on what it missed. + +### `Server.AIOutputChanged` + +Sent each time an AI feature's output for a PR changes as [`GetAIOutput`](#rpchandlergetaioutput) reports it: once when a run is claimed (it reads `pending` from then on, while it waits for an automatic-run slot included), and once when its result is stored. Every run is announced, whoever started it — [`RunAIFeature`](#rpchandlerrunaifeature), opening a PR (the applied features), or a workflow cycle (the automatic ones). A request that starts nothing (`up-to-date`, `already-running`) announces nothing. A client showing a feature's result reads `GetAIOutput` again when one names that PR and feature, instead of polling it. + +```json +{"method": "Server.AIOutputChanged", "params": [{"owner": "C-Hipple", "repo": "code-review-server", "number": 303, "feature": "comments-addressed", "status": "success"}], "id": null} +``` + +| Field | Type | Description | +|-----------|--------|-------------------------------------------------------------------------------------------------| +| `owner` | string | Repository owner, as the run was asked for it | +| `repo` | string | Repository name | +| `number` | int | Pull request number | +| `feature` | string | The feature ID | +| `status` | string | `pending` as a run starts; the stored result's status (`success`, `error`, `insufficient-input`) as it lands | + +As with plugins, a client that (re)subscribes reads `GetAIOutput` once to catch up. A run's in-flight state lives in the server's memory, so a run a server restart lost is never announced as landing; the new server reads the feature as its stored result, not `pending`. + --- ## Methods @@ -392,7 +428,7 @@ Represents a submitted review (e.g. APPROVED, CHANGES_REQUESTED). Fetches a pull request from GitHub and returns it as rendered content (including diff, comments, conversations). Cached data is used where available; see `SyncPR` for a forced refresh. -This is a read-only query as far as the caller is concerned: it never blocks on plugin execution or LLM analysis. Those are dispatched in the background, and their results arrive through `GetPluginOutput` (or on a later `GetPR`'s `annotations`). +This is a read-only query as far as the caller is concerned: it never blocks on plugin execution or LLM analysis. Those are dispatched in the background, and their results arrive through `GetPluginOutput` (or on a later `GetPR`'s `annotations`); a subscribed client hears a [`Server.PluginOutputChanged`](#serverpluginoutputchanged) as each plugin's result lands. **Arguments** (`GetPRstructArgs`): | Field | Type | Required | Description | @@ -837,7 +873,7 @@ Retrieves the output and status of all plugins for a specific pull request. Clears cached plugin results for a pull request and runs the plugins again, bypassing the head-SHA cache check that normally makes a plugin skip a PR it has already seen. -Execution happens in the background: the reply means the rerun was **accepted**, not that it finished. Poll `GetPluginOutput` for results — a plugin that is still running reports `pending`. +Execution happens in the background: the reply means the rerun was **accepted**, not that it finished. A subscribed client hears a [`Server.PluginOutputChanged`](#serverpluginoutputchanged) for each result the rerun clears (before this reply), and again as each plugin is marked `pending` and as it finishes; `GetPluginOutput` then serves the result. **Arguments** (`RerunPluginsArgs`): | Field | Type | Required | Description | @@ -898,7 +934,7 @@ Lists every registered [AI feature](ai_features.md), enabled or not, as the serv ### `RPCHandler.RunAIFeature` -Asks for an AI feature to run for a pull request and returns at once; the run happens in the background, like [`RerunPlugins`](#rpchandlerrerunplugins). Poll [`GetAIOutput`](#rpchandlergetaioutput) while the output's `status` is `pending`. +Asks for an AI feature to run for a pull request and returns at once; the run happens in the background, like [`RerunPlugins`](#rpchandlerrerunplugins). A subscribed client hears a [`Server.AIOutputChanged`](#serveraioutputchanged) as the run starts (before this reply) and as it lands; [`GetAIOutput`](#rpchandlergetaioutput) then serves the result. Unless `Force` is set, a stored result that already covers the PR's current inputs — the same head SHA and the same [inputs digest](ai_features.md#caching-and-staleness), or just the same head SHA for a feature that reads only the code — answers without a run. Only one run per PR and feature is ever in flight. @@ -923,7 +959,7 @@ Unless `Force` is set, a stored result that already covers the PR's current inpu ### `RPCHandler.GetAIOutput` -Returns stored AI feature results for a pull request. It is a pure read — it never starts a run — so clients can poll it. +Returns stored AI feature results for a pull request. It is a pure read — it never starts a run — so a client reads it whenever a [`Server.AIOutputChanged`](#serveraioutputchanged) names the PR. **Arguments** (`GetAIOutputArgs`): | Field | Type | Required | Description | @@ -959,7 +995,7 @@ The body and annotations follow the [plugin response contract](plugins.md#plugin | `truncated` | bool | `true` when some input was cut to fit the model's prompt | | `updated_at` | string | When the stored result was written (RFC 3339); empty when there is none | -**Example Request** (poll one feature): +**Example Request** (read one feature): ```json { "method": "RPCHandler.GetAIOutput", @@ -1294,8 +1330,8 @@ A typical code review workflow using this API: Alongside that loop: - **Expand context**: `GetHunkContext` fetches lines around a hunk boundary when the diff's own context isn't enough to judge a change. -- **Plugin results**: `GetPluginOutput` polls for plugin results, which arrive asynchronously; `RerunPlugins` forces a re-execution. -- **AI features**: `ListAIFeatures` says which are enabled; `RunAIFeature` starts one for the PR and `GetAIOutput` polls for its result (see [AI Features](ai_features.md)). +- **Plugin results**: `GetPluginOutput` reads the plugin results, which arrive asynchronously, each announced by a [`Server.PluginOutputChanged`](#serverpluginoutputchanged) to a subscribed client; `RerunPlugins` forces a re-execution. +- **AI features**: `ListAIFeatures` says which are enabled; `RunAIFeature` starts one for the PR and `GetAIOutput` reads its result once a [`Server.AIOutputChanged`](#serveraioutputchanged) says it landed (see [AI Features](ai_features.md)). Steps 2–5 each return [the PR payload](#the-pr-payload) in full, so a client can apply the reply directly rather than tracking which parts of its local state a mutation invalidated. diff --git a/docs/web_client_features.md b/docs/web_client_features.md index fea5b7d5..be895f3d 100644 --- a/docs/web_client_features.md +++ b/docs/web_client_features.md @@ -131,7 +131,7 @@ browser cannot. Reproduce them if you want the corresponding features. | `POST /api/prepare-diff-lsp` | Writes the diff plus a `Project/Root/Worktree/Buffer/Type` header to a temp file and returns its path; `diff-lsp` consumes that file. | | `WS /api/lsp?tempfile=` | Connects to a `diff-lsp` for the tempfile's workspace. | | `WS /api/lsp-file?lang=&root=` | Connects to a plain language server (`gopls`, `typescript-language-server`, `rust-analyzer`, `pyright-langserver`, resolved at startup) for `root`. | -| `GET /api/events` | Server-Sent Events: each server notification as an event named after its method (`Server.Heartbeat`, ...), data its params. A new stream opens with the latest heartbeat if the next isn't overdue yet. The connection has no idle timeout. | +| `GET /api/events` | Server-Sent Events: each server notification as an event named after its method (`Server.Heartbeat`, `Server.PluginOutputChanged`, `Server.AIOutputChanged`), data its params. A new stream opens with the latest heartbeat if the next isn't overdue yet. The connection has no idle timeout. | | `POST /api/lsp-lines` | The text on each line a list of LSP locations points at, read from disk, so the popover can show what each reference is. | The WebSocket handlers translate between browser JSON messages and LSP's @@ -457,10 +457,15 @@ Behavior worth reproducing: `GetPluginOutput` again. The server deletes the old result before it accepts the re-run and only marks the plugin `pending` once it gets going, so a re-run plugin missing from the output is shown as `pending` rather than dropped. -- **Polling** (`usePluginOutputs`) — while any plugin reads `pending`, both surfaces - read `GetPluginOutput` again every two seconds, one read at a time, until each - lands; only the latest read's reply is applied. Polling gives up after six minutes - of no change (the server stops a plugin after five). +- **Following runs** (`usePluginOutputs`, `useServerChanges`) — nothing is polled. + The server sends a [`Server.PluginOutputChanged`](protocol.md#serverpluginoutputchanged) + each time a PR's plugin output changes (a re-run clearing a result, a plugin + marked `pending`, a result landing), and both surfaces read `GetPluginOutput` + again when one names their PR, and when the `/api/events` stream reconnects + (whatever was announced while it was down never arrives). Reads go one at a + time — announcements that land mid-read ask for one more — and only the latest + read's reply is applied. A re-run that never lands stops reading as `pending` + after six minutes (the server stops a plugin after five). ### 3.10 Plugin annotations in the diff (`annotation_utils.ts`) @@ -592,7 +597,10 @@ scrolling strip, and row actions stay visible instead of waiting for a hover. `/api/events`, and **Server not responding** once three intervals pass without one or the stream drops, until the next arrives. Nothing shows before the first heartbeat, so a server too old to send them isn't reported as down. Every - listener on the page shares one `EventSource` (`serverEvents.on(method, ...)`). + listener on the page shares one `EventSource` (`serverEvents.on(method, ...)`, + and `serverEvents.onOpen` for each time it (re)connects). The same stream carries + `Server.PluginOutputChanged` and `Server.AIOutputChanged`, which the plugin and AI + views follow instead of polling. --- @@ -620,9 +628,15 @@ Both run and render a report the same way (`useAIReport`, `AIReportView`): the feature's output and calls `RunAIFeature` (not forced) when the feature never ran for the PR (`not-run`) or its result is `stale`; the server answers from its cache when nothing changed. **↻ Re-run** forces a run. -- **Polling** — while the output reads `pending`, the report polls `GetAIOutput` - (every 1.5 s, backing off to 3 s, giving up after six minutes). The previous - result stays on screen, dimmed, until the new one lands. +- **Following runs** — nothing is polled. The server sends a + [`Server.AIOutputChanged`](protocol.md#serveraioutputchanged) as a run of the + feature starts and as it lands, whoever asked for it, and the report reads + `GetAIOutput` again on each (and when the event stream reconnects). Each request + takes a ticket and only a reply newer than the one shown is applied, so a run's + reply written before the run landed can't overwrite the result. The previous + result stays on screen, dimmed, until the new one lands. The review toolbar + (`useAIBackgroundRuns`) follows every other feature's runs the same way, so its + spinners and counts stay current without a report open. - **comments-addressed** renders from the typed `report`: the verdict and summary, a "Needs attention" list (the served `outstanding` list — outstanding, then unclear) with each item's status and whether GitHub or the model decided it, @@ -688,8 +702,8 @@ basics: around a hunk boundary, so a diff can be expanded without fetching the whole file. Powers the ↑/↓ buttons on hunk headers. - [`RerunPlugins`](protocol.md#rpchandlerrerunplugins) — clears cached plugin results - and re-executes. The reply means "accepted", not "finished": poll - `GetPluginOutput` for results. + and re-executes. The reply means "accepted", not "finished": read + `GetPluginOutput` when a `Server.PluginOutputChanged` says the result landed. ### Reply fields worth knowing about @@ -701,7 +715,7 @@ basics: - [`annotations`](protocol.md#the-pr-payload) — plugin annotations aggregated server-side. The client derives its own from the plugin outputs instead (see [3.10](#310-plugin-annotations-in-the-diff-annotation_utilsts)), but this field is - there for clients that don't poll plugins separately. + there for clients that don't read plugin output separately. - [`comments[].review_id`](protocol.md#comment-object-commentjson) — ties a comment to the review that submitted it, which is what makes the discussion timeline in [3.4](#34-pr-discussion-prdiscussiontsx-discussion_utilsts) possible. @@ -730,7 +744,8 @@ Ordered roughly by how much time they'll cost you if missed. `C-Hipple/code-review-server` — see the Cache Key Convention in the project's `CLAUDE.md`. Full names miss the DB caches and hit GitHub every time. 3. **Nothing on the read path blocks on plugins or the LLM.** `GetPR` returns - immediately with whatever is cached; plugin output arrives via separate polling. + immediately with whatever is cached; plugin output and AI results arrive later, + each announced by a server notification. Design your UI for results that show up late, and consider the list-view warm-up trick in [3.1](#31-review-list-prlisttsx). 4. **Mutations return the whole PR.** `AddComment`, `EditComment`, `DeleteComment`, @@ -804,7 +819,8 @@ PR-specific parsing. 3. **Write path.** `AddComment` / `EditComment` / `DeleteComment` → `SubmitReview`, plus `SyncPR` afterward. 4. **Navigation.** `GetAdjacentPR`, URL state, deep links. -5. **Plugins.** `GetPluginOutput` polling, the body contract, `RerunPlugins`. +5. **Plugins.** `GetPluginOutput` (read again on `Server.PluginOutputChanged`), the + body contract, `RerunPlugins`. 6. **Extras.** Annotations in the diff, hunk expansion, the config editor, LSP, the code viewer, AI reports (`ListAIFeatures`, `RunAIFeature`, `GetAIOutput`). diff --git a/server/ai_test.go b/server/ai_test.go index de6dcfc3..5ec303ac 100644 --- a/server/ai_test.go +++ b/server/ai_test.go @@ -819,3 +819,69 @@ func TestFeatureFlagsEndToEnd(t *testing.T) { } } } + +// aiChange reads the next line written as a Server.AIOutputChanged. +func aiChange(t *testing.T, out <-chan string) AIOutputChanged { + t.Helper() + line := nextLine(t, out, 5*time.Second) + var n struct { + Method string `json:"method"` + Params []AIOutputChanged `json:"params"` + ID any `json:"id"` + } + if err := json.Unmarshal([]byte(line), &n); err != nil { + t.Fatalf("not JSON: %s: %v", line, err) + } + if n.Method != AIOutputChangedMethod || n.ID != nil || len(n.Params) != 1 { + t.Fatalf("not a %s notification: %s", AIOutputChangedMethod, line) + } + return n.Params[0] +} + +func TestRunAIFeatureAnnouncesTheRunToASubscribedClient(t *testing.T) { + db := aiTestSetup(t, []config.AIFeature{{ID: "stub", Enabled: true}}) + seedAIPR(t, db, aiTestComments(), aiThreadsJSON(false)) + f := &stubFeature{id: "stub", release: make(chan struct{})} + useAIRunner(t, f).OnChange = notifyAIOutputChanged + out := subscribedNotifier(t) + h := &RPCHandler{} + + // Each announcement says what a client reading GetAIOutput on hearing it + // sees. + read := func() string { + t.Helper() + var reply GetAIOutputReply + if err := h.GetAIOutput(&GetAIOutputArgs{Owner: aiOwner, Repo: aiRepo, Number: aiNumber, Feature: "stub"}, &reply); err != nil { + t.Fatalf("GetAIOutput: %v", err) + } + return reply.Output["stub"].Status + } + + var run RunAIFeatureReply + if err := h.RunAIFeature(&RunAIFeatureArgs{Owner: aiOwner, Repo: aiRepo, Number: aiNumber, Feature: "stub"}, &run); err != nil { + t.Fatalf("RunAIFeature: %v", err) + } + want := AIOutputChanged{Owner: aiOwner, Repo: aiRepo, Number: aiNumber, Feature: "stub", Status: ai.StatusPending} + if got := aiChange(t, out); got != want { + t.Errorf("first announcement = %+v, want %+v", got, want) + } + if got := read(); got != ai.StatusPending { + t.Errorf("read on hearing pending = %q", got) + } + + close(f.release) + want.Status = ai.StatusSuccess + if got := aiChange(t, out); got != want { + t.Errorf("second announcement = %+v, want %+v", got, want) + } + if got := read(); got != ai.StatusSuccess { + t.Errorf("read on hearing success = %q", got) + } + + // Asking again for unchanged inputs runs nothing, so announces nothing. + h.RunAIFeature(&RunAIFeatureArgs{Owner: aiOwner, Repo: aiRepo, Number: aiNumber, Feature: "stub"}, &run) + if run.Outcome != string(ai.OutcomeUpToDate) { + t.Fatalf("outcome = %q, want up-to-date", run.Outcome) + } + expectSilence(t, out, 100*time.Millisecond) +} diff --git a/server/notify.go b/server/notify.go index 58945f1a..ca018524 100644 --- a/server/notify.go +++ b/server/notify.go @@ -18,13 +18,23 @@ import ( // that never subscribes never sees one, so a client that reads exactly one // line per call (the TUI) is unaffected. // -// The one notification so far is the heartbeat, sent every interval while a -// client is subscribed; Notifier.Notify is the seam the next event goes -// through. +// The heartbeat is sent every interval while a client is subscribed; events +// go through Notifier.Notify as they happen: Server.PluginOutputChanged and +// Server.AIOutputChanged, which let a client showing a PR's plugin output or +// AI feature results read them again when they change instead of polling. // HeartbeatMethod is the method a heartbeat notification names. const HeartbeatMethod = "Server.Heartbeat" +// PluginOutputChangedMethod is the method a notification names when a PR's +// stored plugin output changes, so what GetPluginOutput returns for it has. +const PluginOutputChangedMethod = "Server.PluginOutputChanged" + +// AIOutputChangedMethod is the method a notification names when an AI +// feature's output for a PR changes as GetAIOutput reports it: a run starts, +// or its result is stored. +const AIOutputChangedMethod = "Server.AIOutputChanged" + const ( defaultHeartbeatInterval = 10 * time.Second minHeartbeatInterval = time.Second @@ -62,6 +72,70 @@ type Heartbeat struct { IntervalSeconds int `json:"interval_seconds"` } +// PluginOutputChanged is a Server.PluginOutputChanged notification's params. +// It names what changed rather than carrying the output, which can be large: +// a client showing the PR reads GetPluginOutput again. +type PluginOutputChanged struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + Number int `json:"number"` + // Plugin is the plugin whose result changed; empty when every plugin's + // result for the PR was cleared at once, for a rerun of them all. + Plugin string `json:"plugin"` + // Status is the plugin's status as GetPluginOutput now reports it + // (pending, success, error or deferred), or empty when its result was + // cleared for a rerun: the plugin is missing from the output until the + // run marks it pending. + Status string `json:"status"` +} + +// notifyPluginOutputChanged tells a subscribed client that a PR's plugin +// output changed. Without the stdio connection (a one-off command) or a +// subscribed client it does nothing. +func notifyPluginOutputChanged(owner, repo string, number int, plugin, status string) { + n := notifier + if n == nil { + return + } + n.Notify(PluginOutputChangedMethod, PluginOutputChanged{ + Owner: owner, + Repo: repo, + Number: number, + Plugin: plugin, + Status: status, + }) +} + +// AIOutputChanged is a Server.AIOutputChanged notification's params. Like +// PluginOutputChanged it names what changed; a client showing the feature +// reads GetAIOutput again. +type AIOutputChanged struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + Number int `json:"number"` + Feature string `json:"feature"` + // Status is the feature's status as GetAIOutput now reports it: pending + // once a run is claimed (queued for a slot included), then the stored + // result's (success, error or insufficient-input) once it lands. + Status string `json:"status"` +} + +// notifyAIOutputChanged tells a subscribed client that an AI feature's output +// for a PR changed. It is the AI runner's OnChange in server mode. +func notifyAIOutputChanged(owner, repo string, number int, feature, status string) { + n := notifier + if n == nil { + return + } + n.Notify(AIOutputChangedMethod, AIOutputChanged{ + Owner: owner, + Repo: repo, + Number: number, + Feature: feature, + Status: status, + }) +} + // notification is a JSON-RPC 1.0 notification as it goes on the wire. type notification struct { Method string `json:"method"` diff --git a/server/plugins.go b/server/plugins.go index 8d4d1bb1..1d50a922 100644 --- a/server/plugins.go +++ b/server/plugins.go @@ -81,7 +81,7 @@ func RunPluginsForce(owner, repo string, number int, sha string, diff string, co // Skip on-demand plugins unless explicitly requested by name if plugin.OnlyOnDemand && !pluginMap[plugin.Name] { // Record a "deferred" status so clients know this plugin exists but hasn't been requested yet - if err := config.C().DB.UpsertPluginResult(owner, repo, number, plugin.Name, "", "deferred", sha); err != nil { + if err := storePluginResult(owner, repo, number, plugin.Name, "", "deferred", sha); err != nil { slog.Error("Failed to record deferred plugin status", "plugin", plugin.Name, "error", err) } continue @@ -130,7 +130,7 @@ func executePlugin(plugin config.Plugin, owner, repo string, number int, sha str } // Set status to pending - err = config.C().DB.UpsertPluginResult(owner, repo, number, plugin.Name, "", "pending", sha) + err = storePluginResult(owner, repo, number, plugin.Name, "", "pending", sha) if err != nil { slog.Error("Failed to set plugin status to pending", "plugin", plugin.Name, "error", err) } @@ -168,7 +168,7 @@ func executePlugin(plugin config.Plugin, owner, repo string, number int, sha str } if err != nil { slog.Error("Plugin execution failed", "plugin", plugin.Name, "error", err, "call_type", callType, "stderr", out.Stderr) - if upsertErr := config.C().DB.UpsertPluginResult(owner, repo, number, plugin.Name, fmt.Sprintf("Error: %v\nStderr: %s\nStdout: %s", err, out.Stderr, resultStr), "error", sha); upsertErr != nil { + if upsertErr := storePluginResult(owner, repo, number, plugin.Name, fmt.Sprintf("Error: %v\nStderr: %s\nStdout: %s", err, out.Stderr, resultStr), "error", sha); upsertErr != nil { slog.Error("Failed to store plugin error result", "plugin", plugin.Name, "error", upsertErr) } return @@ -177,8 +177,19 @@ func executePlugin(plugin config.Plugin, owner, repo string, number int, sha str slog.Info("Plugin executed", "plugin", plugin.Name, "result_len", len(resultStr), "sha", sha, "call_type", callType) // Store result - err = config.C().DB.UpsertPluginResult(owner, repo, number, plugin.Name, resultStr, "success", sha) + err = storePluginResult(owner, repo, number, plugin.Name, resultStr, "success", sha) if err != nil { slog.Error("Failed to store plugin result", "plugin", plugin.Name, "error", err) } } + +// storePluginResult writes a plugin's result for a PR and tells a subscribed +// client its plugin output changed. Every write of a result goes through +// here, so a client showing the output never has to poll for it. +func storePluginResult(owner, repo string, number int, plugin, result, status, sha string) error { + if err := config.C().DB.UpsertPluginResult(owner, repo, number, plugin, result, status, sha); err != nil { + return err + } + notifyPluginOutputChanged(owner, repo, number, plugin, status) + return nil +} diff --git a/server/plugins_test.go b/server/plugins_test.go index 63f3ce51..c477732a 100644 --- a/server/plugins_test.go +++ b/server/plugins_test.go @@ -3,8 +3,10 @@ package server import ( "crs/config" "crs/database" + "encoding/json" "os" "path/filepath" + "reflect" "strings" "testing" "time" @@ -352,3 +354,201 @@ func TestRunPlugins_StoresAFailedRunWithItsOutput(t *testing.T) { } } } + +// subscribedNotifier stands a subscribed notifier in for the stdio +// connection's for the rest of the test, and returns what it writes. +func subscribedNotifier(t *testing.T) <-chan string { + t.Helper() + w, out := lines(t) + saved := notifier + notifier = newNotifier(w) + notifier.Subscribe(time.Hour) + t.Cleanup(func() { + notifier.Unsubscribe() + notifier = saved + }) + return out +} + +// pluginStatuses collects the Server.PluginOutputChanged notifications for +// owner/repo#1 written until the output has been quiet for `quiet`, as each +// plugin's statuses in order. +func pluginStatuses(t *testing.T, out <-chan string, quiet time.Duration) map[string][]string { + t.Helper() + return prPluginStatuses(t, out, quiet, "owner", "repo", 1) +} + +func prPluginStatuses(t *testing.T, out <-chan string, quiet time.Duration, owner, repo string, number int) map[string][]string { + t.Helper() + got := map[string][]string{} + for { + var line string + select { + case line = <-out: + case <-time.After(quiet): + return got + } + var n struct { + Method string `json:"method"` + Params []PluginOutputChanged `json:"params"` + ID any `json:"id"` + } + if err := json.Unmarshal([]byte(line), &n); err != nil { + t.Fatalf("not JSON: %s: %v", line, err) + } + if n.Method != PluginOutputChangedMethod || n.ID != nil || len(n.Params) != 1 { + t.Fatalf("not a %s notification: %s", PluginOutputChangedMethod, line) + } + p := n.Params[0] + if p.Owner != owner || p.Repo != repo || p.Number != number { + t.Errorf("notification names %s/%s#%d, want %s/%s#%d", p.Owner, p.Repo, p.Number, owner, repo, number) + } + got[p.Plugin] = append(got[p.Plugin], p.Status) + } +} + +func TestRunPlugins_AnnouncesEachResultItStores(t *testing.T) { + db := setupTestDB(t) + dir := t.TempDir() + okScript := filepath.Join(dir, "ok.sh") + os.WriteFile(okScript, []byte("#!/bin/sh\necho fine\n"), 0755) + brokenScript := filepath.Join(dir, "broken.sh") + os.WriteFile(brokenScript, []byte("#!/bin/sh\nexit 3\n"), 0755) + config.SetC(config.Config{ + DB: db, + Plugins: []config.Plugin{ + {Name: "ok", Command: okScript}, + {Name: "broken", Command: brokenScript}, + {Name: "expensive", Command: okScript, OnlyOnDemand: true}, + }, + }) + out := subscribedNotifier(t) + + RunPlugins("owner", "repo", 1, "sha123", "", "", "", "main") + + got := pluginStatuses(t, out, 100*time.Millisecond) + want := map[string][]string{ + "ok": {"pending", "success"}, + "broken": {"pending", "error"}, + "expensive": {"deferred"}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("announced %v, want %v", got, want) + } + + // Each announcement follows its write: what it says is what a client + // reading the output now sees. + results, err := db.GetPluginResults("owner", "repo", 1) + if err != nil { + t.Fatalf("failed to get plugin results: %v", err) + } + for name, statuses := range want { + if last := statuses[len(statuses)-1]; results[name].Status != last { + t.Errorf("%s is stored %q, announced %q", name, results[name].Status, last) + } + } +} + +func TestRunPlugins_AnnouncesNothingWhenNoRunIsNeeded(t *testing.T) { + db := setupTestDB(t) + dir := t.TempDir() + script := filepath.Join(dir, "ok.sh") + os.WriteFile(script, []byte("#!/bin/sh\necho fine\n"), 0755) + config.SetC(config.Config{ + DB: db, + Plugins: []config.Plugin{{Name: "ok", Command: script}}, + }) + if err := db.UpsertPluginResult("owner", "repo", 1, "ok", "fine", "success", "sha123"); err != nil { + t.Fatal(err) + } + out := subscribedNotifier(t) + + // The stored result already covers this head, so nothing is written and + // a client has nothing to read again. + RunPlugins("owner", "repo", 1, "sha123", "", "", "", "main") + + if got := pluginStatuses(t, out, 100*time.Millisecond); len(got) != 0 { + t.Errorf("announced %v for a run that was skipped", got) + } +} + +func TestRunPluginsForce_AnnouncesTheRerun(t *testing.T) { + db := setupTestDB(t) + dir := t.TempDir() + script := filepath.Join(dir, "ok.sh") + os.WriteFile(script, []byte("#!/bin/sh\necho again\n"), 0755) + config.SetC(config.Config{ + DB: db, + Plugins: []config.Plugin{{Name: "ok", Command: script}}, + }) + if err := db.UpsertPluginResult("owner", "repo", 1, "ok", "fine", "success", "sha123"); err != nil { + t.Fatal(err) + } + out := subscribedNotifier(t) + + RunPluginsForce("owner", "repo", 1, "sha123", "", "", "", "main", true, []string{"ok"}) + + want := map[string][]string{"ok": {"pending", "success"}} + if got := pluginStatuses(t, out, 100*time.Millisecond); !reflect.DeepEqual(got, want) { + t.Errorf("announced %v, want %v", got, want) + } +} + +func TestRerunPlugins_AnnouncesTheClearedResultThenTheRun(t *testing.T) { + tests := []struct { + name string + plugins []string + want map[string][]string + }{ + { + name: "one plugin", + plugins: []string{"ok"}, + want: map[string][]string{"ok": {"", "pending", "success"}}, + }, + { + // Every result is cleared at once, then each plugin is announced + // as its rerun goes. + name: "every plugin", + want: map[string][]string{ + "": {""}, + "ok": {"pending", "success"}, + "expensive": {"deferred"}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("CRS_HOME", t.TempDir()) + t.Setenv("CRS_GITHUB_TOKEN", "test-token") + db := setupTestDB(t) + script := filepath.Join(t.TempDir(), "ok.sh") + os.WriteFile(script, []byte("#!/bin/sh\necho again\n"), 0755) + config.SetC(config.Config{ + DB: db, + SleepDuration: 10 * time.Minute, + Plugins: []config.Plugin{ + {Name: "ok", Command: script}, + {Name: "expensive", Command: script, OnlyOnDemand: true}, + }, + }) + t.Cleanup(func() { config.SetC(config.Config{}) }) + // The rerun reads its inputs from the caches, never GitHub. + seedAIPR(t, db, aiTestComments(), aiThreadsJSON(false)) + if err := db.UpsertPluginResult(aiOwner, aiRepo, aiNumber, "ok", "fine", "success", "sha-1"); err != nil { + t.Fatal(err) + } + out := subscribedNotifier(t) + + var reply RerunPluginsReply + args := &RerunPluginsArgs{Owner: aiOwner, Repo: aiRepo, Number: aiNumber, Plugins: tt.plugins} + if err := (&RPCHandler{}).RerunPlugins(args, &reply); err != nil || !reply.Okay { + t.Fatalf("RerunPlugins = %v, okay %v", err, reply.Okay) + } + + got := prPluginStatuses(t, out, time.Second, aiOwner, aiRepo, aiNumber) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("announced %v, want %v", got, tt.want) + } + }) + } +} diff --git a/server/server.go b/server/server.go index f8a09e77..428e3540 100644 --- a/server/server.go +++ b/server/server.go @@ -39,6 +39,8 @@ func RunServer() { // Replies and notifications share stdout; out keeps their lines whole. out := &syncWriter{w: os.Stdout} notifier = newNotifier(out) + // A subscribed client hears each AI run start and land. + aiRunner.OnChange = notifyAIOutputChanged server.ServeCodec(jsonrpc.NewServerCodec(&Stdio{out: out})) } @@ -909,7 +911,8 @@ func (h *RPCHandler) ListPlugins(args *ListPluginsArgs, reply *ListPluginsReply) // PR, keyed by the head SHA plus a digest of the PR's discussion (the head SHA // alone for a feature that reads only the code, see ai.KeyDigest). A client // lists the features, asks for a run — which happens in the background, like a -// plugin rerun — and polls GetAIOutput while the output reads "pending". +// plugin rerun — and reads GetAIOutput again when a Server.AIOutputChanged +// notification says the run started or landed. type ListAIFeaturesArgs struct{} @@ -1031,7 +1034,8 @@ type AIFeatureOutput struct { } // GetAIOutput returns stored AI feature results for a PR. It is a pure read: -// it never starts a run, so clients can poll it. +// it never starts a run, so a client can read it whenever a +// Server.AIOutputChanged notification names the PR. func (h *RPCHandler) GetAIOutput(args *GetAIOutputArgs, reply *GetAIOutputReply) error { var features []ai.Feature if args.Feature != "" { @@ -1847,6 +1851,7 @@ func (h *RPCHandler) RerunPlugins(args *RerunPluginsArgs, reply *RerunPluginsRep slog.Error("Error clearing plugin results", "error", err) return err } + notifyPluginOutputChanged(args.Owner, args.Repo, args.Number, "", "") slog.Info("Cleared all plugin results for PR, triggering rerun", "repo", args.Repo, "pr", args.Number) } else { // Clear only specified plugin results @@ -1856,13 +1861,16 @@ func (h *RPCHandler) RerunPlugins(args *RerunPluginsArgs, reply *RerunPluginsRep slog.Error("Error clearing plugin result", "plugin", pluginName, "error", err) return err } + notifyPluginOutputChanged(args.Owner, args.Repo, args.Number, pluginName, "") } slog.Info("Cleared specific plugin results for PR, triggering rerun", "repo", args.Repo, "pr", args.Number, "plugins", args.Plugins) } // Gather the plugin inputs and run them, all in the background: fetching // the details can reach GitHub on a cache miss, and the caller only needs - // to know the rerun was accepted. Results arrive through GetPluginOutput. + // to know the rerun was accepted. Each result is announced with a + // Server.PluginOutputChanged notification as it lands; GetPluginOutput + // serves it. go func() { details, err := GetPRDetails(args.Owner, args.Repo, args.Number, false) if err != nil { From 8adc3d5918d9cb13c25dbbe611a0880199905319 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 26 Oct 2026 13:21:32 +0000 Subject: [PATCH 2/5] chrome_extension: follow the server's change events instead of polling The service worker now subscribes each new server (RPCHandler.Subscribe, with the slowest heartbeat) before anything else it asks it. The native protocol recognises notifications, whole or chunked, and the service worker relays Server.PluginOutputChanged and Server.AIOutputChanged to the extension pages listening on a `crs-events` port, refusing a port from anything but the extension's own pages, as for RPC. A new server, or a page's port dropping and reopening, tells the page to resync. PRTools no longer polls: it reads plugin or AI output again when an event names its PR, one read of each kind at a time (coalesce), and everything on a resync. The ten-second "just requested" grace and poll_utils go; only the latest read of a kind is applied, a run's reply isn't applied over a read that started after it, and a rerun missing from the output reads as running until a read shows it landed. The content script no longer re-asks for the file order every few seconds: the service worker tells a PR's tabs when a file-ordering result lands (crs-file-order-changed) and they ask once. The e2e fake answers Subscribe, announces each run as it starts and lands, and lands runs from the scenario by itself rather than on a read; a new serverRuns scenario has it start runs on its own. New specs cover a run the server started, nothing read while one is pending, and a newly landed file order laid over an open Files changed view. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011prZsf8nHYe7e2Ke1rrq6z --- chrome_extension/README.md | 69 +++++--- chrome_extension/e2e/README.md | 11 +- chrome_extension/e2e/fixtures/data.ts | 6 + chrome_extension/e2e/harness/fake_crs.ts | 117 ++++++++++--- chrome_extension/e2e/harness/scenario.ts | 12 +- chrome_extension/e2e/harness/test.ts | 7 +- chrome_extension/e2e/pr_order.e2e.ts | 15 +- chrome_extension/e2e/runs.e2e.ts | 39 ++++- chrome_extension/src/background.test.ts | 155 ++++++++++++++++++ chrome_extension/src/background.ts | 79 ++++++++- chrome_extension/src/content.ts | 32 ++-- chrome_extension/src/native_client.test.ts | 97 ++++++++++- chrome_extension/src/native_client.ts | 52 +++++- chrome_extension/src/native_protocol.test.ts | 14 ++ chrome_extension/src/native_protocol.ts | 24 ++- chrome_extension/src/panel/PRList.tsx | 2 +- chrome_extension/src/panel/PRTools.tsx | 141 +++++++++------- chrome_extension/src/panel/coalesce.test.ts | 57 +++++++ chrome_extension/src/panel/coalesce.ts | 34 ++++ .../src/panel/plugin_utils.test.ts | 26 +++ chrome_extension/src/panel/plugin_utils.ts | 39 +++++ chrome_extension/src/panel/poll_utils.test.ts | 34 ---- chrome_extension/src/panel/poll_utils.ts | 38 ----- chrome_extension/src/panel/useNow.ts | 11 ++ chrome_extension/src/panel/usePRChanges.ts | 75 +++++++++ chrome_extension/src/panel/usePolling.ts | 49 ------ chrome_extension/src/server_events.test.ts | 88 ++++++++++ chrome_extension/src/server_events.ts | 86 ++++++++++ chrome_extension/src/types.ts | 51 ++++++ docs/ai_features.md | 3 +- docs/clients.md | 2 +- docs/plugins.md | 2 +- 32 files changed, 1203 insertions(+), 264 deletions(-) create mode 100644 chrome_extension/src/panel/coalesce.test.ts create mode 100644 chrome_extension/src/panel/coalesce.ts delete mode 100644 chrome_extension/src/panel/poll_utils.test.ts delete mode 100644 chrome_extension/src/panel/poll_utils.ts create mode 100644 chrome_extension/src/panel/useNow.ts create mode 100644 chrome_extension/src/panel/usePRChanges.ts delete mode 100644 chrome_extension/src/panel/usePolling.ts create mode 100644 chrome_extension/src/server_events.test.ts create mode 100644 chrome_extension/src/server_events.ts diff --git a/chrome_extension/README.md b/chrome_extension/README.md index 5ac15bde..01d4b46f 100644 --- a/chrome_extension/README.md +++ b/chrome_extension/README.md @@ -172,9 +172,11 @@ the reader scrolls, so a re-render doesn't undo it. It comes from one more read-only message, `crs-file-order` — `GetAIOutput` for `file-ordering`, through the service worker, answered for the sender tab's own -PR — so opening a page never starts a run. A page that finds no order stored yet -asks again every few seconds, so one the server works out just after the page -loaded arrives without a reload. +PR — so opening a page never starts a run. When the server lands a new order for +the PR (one it works out just after the page loaded, or after a push), the +service worker hears it and tells that PR's tabs (`crs-file-order-changed`), and +each asks again: the new order arrives without a reload, and nothing asks on a +timer. The panel follows the system's light or dark setting, not GitHub's theme. The dot in its header says whether the server is connected; hovering it shows the @@ -243,11 +245,12 @@ script and is styled to the panel's theme. Annotations are listed by file and line, each linking to its line in GitHub's Files changed tab (`…/pull/N/files#diff-R`). -While anything reads as running, and for ten seconds after the PR is opened or -a run is requested (a run is dispatched in the background, so the first read -can come before it is marked), the view polls `GetAIOutput` and -`GetPluginOutput` every three seconds. Whether each card is expanded is -remembered. +Nothing is polled. The server announces every run as it starts and as it lands +(see [Change events](#change-events)), whoever started it — this view, another +client, or the server itself after a push — and the view reads `GetAIOutput` or +`GetPluginOutput` again when one names its PR, one read of each at a time. A run +the view asked for reads as running from the moment it is accepted. Whether +each card is expanded is remembered. ## Architecture @@ -259,7 +262,7 @@ github.com tab │ chrome.runtime.sendMessage({type: 'crs-rpc', method, params}) ▼ background.js the service worker: sender checks, RPC_METHODS, call ids, - │ timeouts, chunk reassembly + │ timeouts, chunk reassembly, relaying change events │ one native messaging port: a 4-byte length, then JSON, per message ▼ crs_native_host bridges the two framings, chunks large responses @@ -304,6 +307,21 @@ result as the response. The host also reports on itself in `crs_host` messages: server has started, `error` when it can't be found or started, and `server_exited`, with the server's last lines of output, when it dies. +**Change events.** The server speaks unasked once a client calls +`RPCHandler.Subscribe` ([Server Notifications](../docs/protocol.md#server-notifications)): +it writes JSON-RPC notifications (`{method, params, id: null}`) to the same +stdout as its replies, and the host forwards them like any other line. The +service worker subscribes each new server before anything else it asks it +(with the slowest heartbeat, which it has no use for) and relays two +notifications to the extension pages listening on a `crs-events` port +(`chrome.runtime.connect`): `Server.PluginOutputChanged` and +`Server.AIOutputChanged`, each naming a PR and the plugin or feature that +changed, never the output. A page showing that PR reads the output again. When +a new server has subscribed, or a page's port drops and is opened again, the +page is told to `resync` — read everything again — since whatever happened in +between was never announced. A landed `file-ordering` result also goes, as a +bare `crs-file-order-changed`, to the content script of each tab on that PR. + **What the panel may call.** The service worker forwards only the methods in `RPC_METHODS` (`src/types.ts`): `Hello`, `GetAllReviews`, `GetPR`, `SyncPR`, `ListPlugins`, `GetPluginOutput`, `RerunPlugins`, `ListAIFeatures`, @@ -328,8 +346,10 @@ GitHub's page can't script into it, and it talks to the service worker directly, never through the page. `panel.html` and its files are web-accessible to `https://github.com/*` only. The trust checks: -- The service worker answers only the extension's own pages for RPC and status: - the sender must carry the extension's ID and a `chrome-extension:///` URL. +- The service worker answers only the extension's own pages for RPC, status + and change events: the sender must carry the extension's ID and a + `chrome-extension:///` URL. A `crs-events` port from anything else is + disconnected. A content script shares the ID but reports the GitHub page's URL, so nothing a page can influence reaches the server — its own messages, `crs-tools` and `crs-file-order`, are answered for the tab's own PR and nothing else. @@ -410,19 +430,20 @@ To try a change, rebuild and click the extension's reload button on script predates the reload gets a fresh copy injected when the button is pressed. To inspect the panel, right-click inside the modal and choose Inspect. -| Path | What it is | -| ---------------------------- | ----------------------------------------------------------------------------------------- | -| `manifest.json` | The MV3 manifest, copied into `dist/` | -| `src/background.ts` | The service worker: RPC for extension pages, the toolbar button, the per-tab button title | -| `src/native_client.ts` | The native port: request IDs, timeouts, reconnecting | -| `src/native_protocol.ts` | The host's messages, chunk reassembly and the error kinds (pure, unit tested) | -| `src/content.ts` | The modal on github.com, the chips beside a PR's tabs, the file order over its diffs | -| `src/tool_chips.ts` | Which chips a PR page draws: an enabled feature and a plugin each (pure, unit tested) | -| `src/file_order.ts` | Laying file-ordering's order over GitHub's Files changed view (e2e tested) | -| `src/github_url.ts` | Recognising PR URLs and building diff links | -| `src/rpc.ts`, `src/types.ts` | The panel's typed RPC client, the wire shapes and `RPC_METHODS` | -| `src/panel/` | The React panel: `App.tsx` routes between `PRList.tsx` and `PRTools.tsx` | -| `crs_native_host/` | The native host (Go, standard library only) and `install.sh` | +| Path | What it is | +| ---------------------------- | ----------------------------------------------------------------------------------------------------------- | +| `manifest.json` | The MV3 manifest, copied into `dist/` | +| `src/background.ts` | The service worker: RPC and change events for extension pages, the toolbar button, the per-tab button title | +| `src/native_client.ts` | The native port: request IDs, timeouts, reconnecting, subscribing each new server | +| `src/server_events.ts` | The server's change events as pages hear them (pure, unit tested) | +| `src/native_protocol.ts` | The host's messages, chunk reassembly and the error kinds (pure, unit tested) | +| `src/content.ts` | The modal on github.com, the chips beside a PR's tabs, the file order over its diffs | +| `src/tool_chips.ts` | Which chips a PR page draws: an enabled feature and a plugin each (pure, unit tested) | +| `src/file_order.ts` | Laying file-ordering's order over GitHub's Files changed view (e2e tested) | +| `src/github_url.ts` | Recognising PR URLs and building diff links | +| `src/rpc.ts`, `src/types.ts` | The panel's typed RPC client, the wire shapes and `RPC_METHODS` | +| `src/panel/` | The React panel: `App.tsx` routes between `PRList.tsx` and `PRTools.tsx` | +| `crs_native_host/` | The native host (Go, standard library only) and `install.sh` | The host imports nothing from the module, so it builds without cgo or the server's dependencies. It is still part of the module — `go build ./...` builds diff --git a/chrome_extension/e2e/README.md b/chrome_extension/e2e/README.md index c5d9f8d8..fd64eff7 100644 --- a/chrome_extension/e2e/README.md +++ b/chrome_extension/e2e/README.md @@ -44,9 +44,14 @@ tests share nothing and run in parallel. environment. - **Steering the fake** (`harness/scenario.ts`): specs write `scenario.json` in the state directory (`crs.update(...)`) and the fake re-reads it on every - request — e.g. `finishRuns` lets a pending AI or plugin run land, - `exitOn` makes the server die on a method. The fake appends every request to - `calls.jsonl`, which `crs.calls()` / `crs.waitForCall()` read. + request, and every 50 ms for its runs — e.g. `finishRuns` lands a pending AI + or plugin run, `serverRuns` (`crs.startServerRuns`) has the server start runs + for PR42 on its own, `exitOn` makes the server die on a method. Like the + server, the fake answers `Subscribe` and then announces each run as it + starts and lands (`Server.AIOutputChanged`, `Server.PluginOutputChanged`); + the extension reads output again on those and never polls, and nothing lands + on a read. The fake appends every request to `calls.jsonl`, which + `crs.calls()` / `crs.waitForCall()` read. - **The toolbar button**: Playwright can't click browser UI, and headless Chromium doesn't deliver the `Alt+Shift+R` command, so `github.clickToolbarButton` calls `chrome.action.onClicked.dispatch(tab)` in the service worker. That diff --git a/chrome_extension/e2e/fixtures/data.ts b/chrome_extension/e2e/fixtures/data.ts index a05ed9a6..4b20de40 100644 --- a/chrome_extension/e2e/fixtures/data.ts +++ b/chrome_extension/e2e/fixtures/data.ts @@ -282,6 +282,9 @@ export const FILE_ORDER = [ 'docs/api.md', ]; +/** The order a later file-ordering run lands with: the stored one, backwards. */ +export const RERUN_FILE_ORDER = [...FILE_ORDER].reverse(); + export function aiOutput( feature: AIFeature, fields: Partial = {} @@ -349,6 +352,9 @@ export function pr42AIOutputs(): Record { /** What a finished run of `featureId` reports; `run` counts the runs started. */ export function aiRunResult(featureId: string, run: number): Partial { + if (featureId === 'file-ordering') { + return done({ report: { files: RERUN_FILE_ORDER }, updated_at: new Date().toISOString() }); + } return done({ body: { body_type: 'markdown', diff --git a/chrome_extension/e2e/harness/fake_crs.ts b/chrome_extension/e2e/harness/fake_crs.ts index dd295672..8b8d9888 100644 --- a/chrome_extension/e2e/harness/fake_crs.ts +++ b/chrome_extension/e2e/harness/fake_crs.ts @@ -7,9 +7,12 @@ // log, like the server's own logging. // // The specs steer it through files in CRS_E2E_STATE_DIR (see scenario.ts): -// scenario.json is re-read on every request, and every request is appended -// to calls.jsonl before it is answered. Runs (RunAIFeature, RerunPlugins) are -// kept in memory, which lives as long as the host's connection — one test. +// scenario.json is re-read on every request and every few milliseconds, and +// every request is appended to calls.jsonl before it is answered. Runs +// (RunAIFeature, RerunPlugins, and the ones the scenario has the server start +// on its own) are kept in memory, which lives as long as the host's +// connection — one test. Once subscribed, the fake announces each run as it +// starts and as it lands, as the server does; nothing lands on a read. import { appendFileSync, readFileSync } from 'node:fs'; import { join } from 'node:path'; @@ -56,6 +59,8 @@ function scenario(): Scenario { const aiOutputs = new Map>(); const pluginOutputs = new Map>(); +/** Every PR asked about, by key, for the runs on them to land. */ +const prs = new Map(); /** Runs started per PR and feature / plugin, to tell one run's result from the next. */ const runCounts = new Map(); @@ -70,6 +75,7 @@ function prArgs(args: Args): PR { } function aiFor(pr: PR): Record { + prs.set(prKey(pr), pr); let outputs = aiOutputs.get(prKey(pr)); if (!outputs) { outputs = @@ -82,6 +88,7 @@ function aiFor(pr: PR): Record { } function pluginsFor(pr: PR): Record { + prs.set(prKey(pr), pr); let outputs = pluginOutputs.get(prKey(pr)); if (!outputs) { outputs = prKey(pr) === prKey(PR42) ? pr42PluginOutputs() : {}; @@ -97,24 +104,87 @@ function nextRun(pr: PR, name: string): number { return n; } -/** Lands the runs the scenario says are finished. */ -function settleRuns(pr: PR): void { - const finish = new Set(scenario().finishRuns ?? []); +// --- Notifications ------------------------------------------------------------------ + +/** Set by RPCHandler.Subscribe: the server sends no notification before it. */ +let subscribed = false; + +function notify(method: string, params: unknown): void { + if (!subscribed) return; + process.stdout.write(`${JSON.stringify({ method, params: [params], id: null })}\n`); +} + +function aiChanged(pr: PR, feature: string, status: string): void { + notify('Server.AIOutputChanged', { ...pr, feature, status }); +} + +function pluginChanged(pr: PR, plugin: string, status: string): void { + notify('Server.PluginOutputChanged', { ...pr, plugin, status }); +} + +// --- Runs ---------------------------------------------------------------------------- + +const FEATURE_IDS = new Set(FEATURES.map(f => f.id)); + +/** Starts a run of a feature or plugin: pending, and announced, until it lands. */ +function startRun(pr: PR, name: string): void { + nextRun(pr, name); + if (FEATURE_IDS.has(name)) { + const outputs = aiFor(pr); + // A run in flight still carries the previous result, as the server's does. + outputs[name] = { ...outputs[name], status: 'pending' }; + aiChanged(pr, name, 'pending'); + return; + } + pluginsFor(pr)[name] = { + result: '', + status: 'pending', + body: { body_type: 'markdown', body_content: '' }, + annotations: null, + }; + pluginChanged(pr, name, 'pending'); +} + +/** Lands the runs the scenario says are finished, announcing each. */ +function settleRuns(pr: PR, finish: Set): void { const ai = aiFor(pr); for (const [id, output] of Object.entries(ai)) { if (output.status === 'pending' && finish.has(id)) { const run = runCounts.get(`${prKey(pr)}:${id}`) ?? 1; ai[id] = { ...output, ...aiRunResult(id, run), stale: false }; + aiChanged(pr, id, ai[id].status); } } const plugins = pluginsFor(pr); for (const [name, output] of Object.entries(plugins)) { if (output.status === 'pending' && finish.has(name)) { plugins[name] = pluginRunResult(name, runCounts.get(`${prKey(pr)}:${name}`) ?? 1); + pluginChanged(pr, name, plugins[name].status); } } } +/** The runs the scenario had the server start on its own, so each starts once. */ +const serverRunsStarted = new Set(); + +/** + * Does what the scenario asks of runs: starts the ones the server starts on + * its own, and lands the finished ones — by itself, as the server's runs + * land, whether or not anyone reads. + */ +function followScenario(): void { + if (exiting) return; + const current = scenario(); + for (const name of current.serverRuns ?? []) { + if (serverRunsStarted.has(name)) continue; + serverRunsStarted.add(name); + startRun(PR42, name); + } + const finish = new Set(current.finishRuns ?? []); + if (finish.size === 0) return; + for (const pr of prs.values()) settleRuns(pr, finish); +} + // --- Methods --------------------------------------------------------------------- const handlers: Record unknown> = { @@ -129,11 +199,7 @@ const handlers: Record unknown> = { 'RPCHandler.ListPlugins': () => ({ plugins: PLUGINS }), - 'RPCHandler.GetPluginOutput': args => { - const pr = prArgs(args); - settleRuns(pr); - return { output: pluginsFor(pr) }; - }, + 'RPCHandler.GetPluginOutput': args => ({ output: pluginsFor(prArgs(args)) }), // Like the server: clears the named plugins' results (all of them without // `Plugins`) and runs them in the background. @@ -144,13 +210,9 @@ const handlers: Record unknown> = { : PLUGINS.filter(p => !p.OnlyOnDemand).map(p => p.Name); const outputs = pluginsFor(pr); for (const name of names) { - nextRun(pr, name); - outputs[name] = { - result: '', - status: 'pending', - body: { body_type: 'markdown', body_content: '' }, - annotations: null, - }; + delete outputs[name]; + pluginChanged(pr, name, ''); + startRun(pr, name); } return { okay: true, message: `Rerunning ${names.length} plugin(s)`, output: {} }; }, @@ -160,7 +222,6 @@ const handlers: Record unknown> = { // Every enabled feature's output, or just `Feature`'s. 'RPCHandler.GetAIOutput': args => { const pr = prArgs(args); - settleRuns(pr); const all = aiFor(pr); const wanted = args.Feature ? FEATURES.filter(f => f.id === args.Feature) @@ -192,9 +253,7 @@ const handlers: Record unknown> = { if (!args.Force && current.status === 'success' && !current.stale) { return { okay: true, outcome: 'up-to-date', message: '', output: current }; } - nextRun(pr, feature.id); - // A run in flight still carries the previous result, as the server's does. - outputs[feature.id] = { ...current, status: 'pending' }; + startRun(pr, feature.id); return { okay: true, outcome: 'started', @@ -202,6 +261,18 @@ const handlers: Record unknown> = { output: outputs[feature.id], }; }, + + // The extension asks for the slowest heartbeat and never hears one here. + 'RPCHandler.Subscribe': args => { + subscribed = true; + const now = new Date().toISOString(); + return { + seq: 0, + time: now, + started_at: now, + interval_seconds: Number(args.IntervalSeconds) || 10, + }; + }, }; // --- JSON-RPC over stdio --------------------------------------------------------- @@ -242,6 +313,8 @@ function handle(req: Request): void { process.stderr.write(`fake crs: pid ${process.pid}, args ${process.argv.slice(2).join(' ')}\n`); +setInterval(followScenario, 50); + let buffer = ''; process.stdin.setEncoding('utf-8'); process.stdin.on('data', (chunk: string) => { diff --git a/chrome_extension/e2e/harness/scenario.ts b/chrome_extension/e2e/harness/scenario.ts index 06009b61..4a2df70b 100644 --- a/chrome_extension/e2e/harness/scenario.ts +++ b/chrome_extension/e2e/harness/scenario.ts @@ -17,11 +17,17 @@ export interface Scenario { /** GetAllReviews serves the fixture list, or no PRs at all. Default `fixtures`. */ reviews?: 'fixtures' | 'empty'; /** - * AI feature ids and plugin names whose runs in flight land the next time - * their output is read. A run started with RunAIFeature / RerunPlugins - * reads `pending` until its name is listed here. + * AI feature ids and plugin names whose runs in flight land, by + * themselves and at once, announced as the server announces its runs. A + * run reads `pending` until its name is listed here. */ finishRuns?: string[]; + /** + * AI feature ids and plugin names the server starts a run of for PR42 on + * its own (a push, a workflow cycle), once each: nothing in the browser + * asks for them. + */ + serverRuns?: string[]; /** * Exit with `code`, after writing `stderr` to stderr, on receiving * `method` (without answering it) — a server dying under the host. diff --git a/chrome_extension/e2e/harness/test.ts b/chrome_extension/e2e/harness/test.ts index 3ddb8146..acf541c8 100644 --- a/chrome_extension/e2e/harness/test.ts +++ b/chrome_extension/e2e/harness/test.ts @@ -93,11 +93,16 @@ export class FakeCrs { renameSync(`${path}.tmp`, path); } - /** Lets the named runs (AI feature ids, plugin names) land on their next read. */ + /** Lets the named runs (AI feature ids, plugin names) land, announced. */ finishRuns(...names: string[]): void { this.update({ finishRuns: names }); } + /** Has the server start runs of the named features or plugins for PR42 on its own. */ + startServerRuns(...names: string[]): void { + this.update({ serverRuns: names }); + } + /** Every request the fake received, oldest first; `method` without `RPCHandler.`. */ calls(method?: string): RecordedCall[] { const path = join(this.stateDir, CALLS_FILE); diff --git a/chrome_extension/e2e/pr_order.e2e.ts b/chrome_extension/e2e/pr_order.e2e.ts index 2ad7b537..366f3fb0 100644 --- a/chrome_extension/e2e/pr_order.e2e.ts +++ b/chrome_extension/e2e/pr_order.e2e.ts @@ -4,7 +4,7 @@ // where GitHub put them. import type { Page } from '@playwright/test'; -import { FILE_ORDER } from './fixtures/data'; +import { FILE_ORDER, RERUN_FILE_ORDER } from './fixtures/data'; import { expect, test } from './harness/test'; /** A file the fixture report doesn't name: GitHub's order keeps it, so it stays last. */ @@ -70,6 +70,19 @@ test('GitHub re-rendering the diff list brings the stored order back', async ({ await expect.poll(() => diffPaths(page)).toEqual([...FILE_ORDER, ...UNNAMED]); }); +test('a new order the server lands is laid over the open page', async ({ github, crs }) => { + const page = await github.open('/acme/widgets/pull/42/files'); + await expect.poll(() => diffPaths(page)).toEqual([...FILE_ORDER, ...UNNAMED]); + + // file-ordering runs again for the PR (a push, say) and lands another + // order. The service worker hears it land and tells the page, which asks + // for the order once more; it never asks on a timer. + crs.startServerRuns('file-ordering'); + crs.finishRuns('file-ordering'); + await expect.poll(() => diffPaths(page)).toEqual([...RERUN_FILE_ORDER, ...UNNAMED]); + expect(crs.calls('GetAIOutput')).toHaveLength(2); +}); + test('elsewhere on the PR nothing is asked for or moved', async ({ github, crs }) => { const page = await github.open('/acme/widgets/pull/42'); diff --git a/chrome_extension/e2e/runs.e2e.ts b/chrome_extension/e2e/runs.e2e.ts index 8a86a565..5e7031a3 100644 --- a/chrome_extension/e2e/runs.e2e.ts +++ b/chrome_extension/e2e/runs.e2e.ts @@ -1,6 +1,7 @@ // Run / Re-run: AI features through RunAIFeature, plugins through // RerunPlugins, each reading `pending` until the fake lets the run land -// (crs.finishRuns), which the panel picks up by polling. +// (crs.finishRuns). The fake announces each run as it starts and lands, as +// the server does, and the panel reads the output again then: it never polls. import { card, expect, statusChip, test } from './harness/test'; @@ -96,3 +97,39 @@ test('Run an on-demand plugin, then Re-run all', async ({ github, crs }) => { ); await expect(doc.getByRole('heading', { name: 'Style guide findings (run 1)' })).toBeVisible(); }); + +test('runs the server starts on its own show up and land, with nothing polled', async ({ + github, + crs, +}) => { + const page = await github.open('/acme/widgets/pull/42'); + const panel = await github.openPanel(page); + const security = card(panel, 'security_check'); + const flags = card(panel, 'Behind a flag?'); + await expect(statusChip(security)).toHaveText('Success'); + await expect(statusChip(flags)).toHaveText('Not run'); + + // A push, say: nothing in the browser asked for these runs. + crs.startServerRuns('security_check', 'feature-flags'); + await expect(statusChip(security)).toHaveText('Running'); + await expect(statusChip(flags)).toHaveText('Running'); + expect(crs.calls('RerunPlugins')).toEqual([]); + expect(crs.calls('RunAIFeature')).toEqual([]); + + // While they run, the panel asks the server nothing. + const reads = () => crs.calls('GetPluginOutput').length + crs.calls('GetAIOutput').length; + await page.waitForTimeout(500); + const before = reads(); + await page.waitForTimeout(4000); + expect(reads()).toBe(before); + + crs.finishRuns('security_check', 'feature-flags'); + await expect(statusChip(security)).toHaveText('Success'); + await expect(security.locator('.card-summary')).toHaveText( + 'Run 1 of security_check: no new findings.' + ); + await expect(statusChip(flags)).toHaveText('Success'); + await expect(flags.locator('.card-summary')).toHaveText( + 'Run 1: feature-flags found 2 of 5 logic changes outside a flag.' + ); +}); diff --git a/chrome_extension/src/background.test.ts b/chrome_extension/src/background.test.ts index 3ebe3b28..b933ea29 100644 --- a/chrome_extension/src/background.test.ts +++ b/chrome_extension/src/background.test.ts @@ -58,6 +58,29 @@ class FakePort { receive(m: unknown) { for (const cb of this.onMessage.listeners) (cb as (m: unknown) => void)(m); } + drop() { + for (const cb of this.onDisconnect.listeners) (cb as () => void)(); + } +} + +/** A page's events port, as the service worker's onConnect sees it. */ +class FakeEventsPort { + posted: unknown[] = []; + disconnected = false; + onDisconnect = event(); + constructor( + readonly name: string, + readonly sender: object + ) {} + postMessage(m: unknown) { + this.posted.push(m); + } + disconnect() { + this.disconnected = true; + } + close() { + for (const cb of this.onDisconnect.listeners) (cb as () => void)(); + } } const ports: FakePort[] = []; @@ -74,6 +97,7 @@ const fake = { getContexts: mock(async (_filter: unknown): Promise => []), ContextType: { TAB: 'TAB' }, onMessage: event(), + onConnect: event(), onInstalled: event(), onStartup: event(), }, @@ -393,3 +417,134 @@ describe('button title', () => { expect(fake.action.setTitle).not.toHaveBeenCalled(); }); }); + +describe('change events', () => { + /** Opens an events port as `sender` would. */ + function connect(sender: object = EXTENSION_PAGE, name = 'crs-events') { + const port = new FakeEventsPort(name, sender); + for (const cb of fake.runtime.onConnect.listeners) (cb as (p: unknown) => void)(port); + return port; + } + + /** The native port, opened by a call if none is open yet. */ + async function nativePort(): Promise { + const reply = sendAs(EXTENSION_PAGE, { + type: 'crs-rpc', + method: 'RPCHandler.ListPlugins', + params: {}, + }); + const port = ports[ports.length - 1]; + const request = port.sent[port.sent.length - 1] as { id: number }; + port.receive({ id: request.id, result: { plugins: [] }, error: null }); + await reply; + return port; + } + + const PR = { owner: 'o', repo: 'r', number: 1 }; + + test("relays the server's plugin and AI changes to extension pages", async () => { + const native = await nativePort(); + const page = connect(); + native.receive({ + method: 'Server.PluginOutputChanged', + params: [{ ...PR, plugin: 'lint', status: 'success' }], + id: null, + }); + native.receive({ + method: 'Server.AIOutputChanged', + params: [{ ...PR, feature: 'comments-addressed', status: 'pending' }], + id: null, + }); + // Pages have no use for the heartbeat, or a method they don't know. + native.receive({ method: 'Server.Heartbeat', params: [{ seq: 1 }], id: null }); + native.receive({ method: 'Server.Other', params: [PR], id: null }); + + expect(page.posted).toEqual([ + { + type: 'plugin-output-changed', + change: { ...PR, plugin: 'lint', status: 'success' }, + }, + { + type: 'ai-output-changed', + change: { ...PR, feature: 'comments-addressed', status: 'pending' }, + }, + ]); + + // A page that went away hears nothing more. + page.close(); + native.receive({ + method: 'Server.PluginOutputChanged', + params: [{ ...PR, plugin: 'lint', status: 'pending' }], + id: null, + }); + expect(page.posted).toHaveLength(2); + }); + + test('content scripts may not listen', () => { + const port = connect(CONTENT_SCRIPT); + expect(port.disconnected).toBe(true); + // Nor may another extension. + expect(connect({ ...EXTENSION_PAGE, id: 'someoneelse' }).disconnected).toBe(true); + }); + + test('a port of another name is left alone', async () => { + const native = await nativePort(); + const port = connect(EXTENSION_PAGE, 'something-else'); + native.receive({ + method: 'Server.PluginOutputChanged', + params: [{ ...PR, plugin: 'lint', status: 'success' }], + id: null, + }); + expect(port.disconnected).toBe(false); + expect(port.posted).toEqual([]); + }); + + test('a new server tells every page to read again', async () => { + const page = connect(); + (await nativePort()).drop(); + + // The next call starts a new server, subscribed first. + const reply = sendAs(EXTENSION_PAGE, { + type: 'crs-rpc', + method: 'RPCHandler.ListPlugins', + params: {}, + }); + const native = ports[ports.length - 1]; + const [subscribe, call] = native.sent as { id: number; method: string }[]; + expect(subscribe.method).toBe('RPCHandler.Subscribe'); + native.receive({ id: subscribe.id, result: { seq: 0 }, error: null }); + native.receive({ id: call.id, result: { plugins: [] }, error: null }); + await reply; + await settle(); + expect(page.posted).toEqual([{ type: 'resync' }]); + }); + + test("a landed file order tells its PR's tabs to ask again", async () => { + const native = await nativePort(); + fake.tabs.query.mockResolvedValueOnce([ + { id: 1, url: 'https://github.com/o/r/pull/1/files' }, + { id: 2, url: 'https://github.com/O/R/pull/1' }, + { id: 3, url: 'https://github.com/o/r/pull/2/files' }, + { id: 4, url: 'https://github.com/o/r' }, + ]); + native.receive({ + method: 'Server.AIOutputChanged', + params: [{ ...PR, feature: 'file-ordering', status: 'success' }], + id: null, + }); + await settle(); + expect(fake.tabs.sendMessage.mock.calls).toEqual([ + [1, { type: 'crs-file-order-changed' }], + [2, { type: 'crs-file-order-changed' }], + ]); + + // A run starting changes no order yet. + native.receive({ + method: 'Server.AIOutputChanged', + params: [{ ...PR, feature: 'file-ordering', status: 'pending' }], + id: null, + }); + await settle(); + expect(fake.tabs.query).toHaveBeenCalledTimes(1); + }); +}); diff --git a/chrome_extension/src/background.ts b/chrome_extension/src/background.ts index 2c3228dc..51c2f1ab 100644 --- a/chrome_extension/src/background.ts +++ b/chrome_extension/src/background.ts @@ -1,21 +1,26 @@ // Service worker: owns the connection to the server (through the native host, // see native_client.ts), answers extension pages' RPC and status requests, -// and turns a toolbar click into the modal on github.com or a popup window -// anywhere else. +// relays the server's change events to them, and turns a toolbar click into +// the modal on github.com or a popup window anywhere else. import { isGitHubUrl, parsePullUrl, type PullRef } from './github_url'; import { fileOrderingFiles } from './file_order'; import { NativeClient } from './native_client'; -import type { ErrorInfo } from './native_protocol'; +import type { ErrorInfo, JsonRpcNotification } from './native_protocol'; +import { isChangeFor, toServerEvent } from './server_events'; import { chipsFor } from './tool_chips'; import { + EVENTS_PORT, FILE_ORDERING, isRpcMethod, type AIFeature, type AIFeatureOutput, + type AIOutputChanged, + type FileOrderChangedMessage, type FileOrderReply, type PluginConfig, type RpcReply, + type ServerEventMessage, type ToggleMessage, type ToolsReply, } from './types'; @@ -27,6 +32,10 @@ const STANDALONE_PATH = 'panel.html?standalone=1'; const client = new NativeClient({ connect: () => chrome.runtime.connectNative(HOST_NAME), lastError: () => chrome.runtime.lastError?.message, + onNotification, + // A new server: pages read what they show again, since whatever it did + // before it subscribed was never announced. + onSubscribed: () => broadcast({ type: 'resync' }), }); // --- Messages from extension pages ------------------------------------------- @@ -91,6 +100,70 @@ chrome.runtime.onMessage.addListener((message: unknown, sender, sendResponse) => return false; }); +// --- The server's change events ------------------------------------------------- + +/** + * Extension pages listening for change events, each on a port of its own + * (EVENTS_PORT). A page that goes away takes its port with it. + */ +const eventPorts = new Set(); + +chrome.runtime.onConnect.addListener(port => { + if (port.name !== EVENTS_PORT) return; + if (!port.sender || !isExtensionPage(port.sender)) { + port.disconnect(); + return; + } + eventPorts.add(port); + port.onDisconnect.addListener(() => eventPorts.delete(port)); +}); + +function broadcast(message: ServerEventMessage): void { + for (const port of eventPorts) { + try { + port.postMessage(message); + } catch { + // Closed without a disconnect event reaching us yet. + eventPorts.delete(port); + } + } +} + +function onNotification(notification: JsonRpcNotification): void { + const event = toServerEvent(notification); + if (!event) return; + broadcast(event); + if ( + event.type === 'ai-output-changed' && + event.change.feature === FILE_ORDERING && + event.change.status !== 'pending' + ) { + void tellFileOrderChanged(event.change); + } +} + +/** + * A file order landed for a PR: tells the content script on each of its tabs, + * which asks for it again (fileOrderFor) and lays it over a Files changed + * view. The message says only that, never the order. + */ +async function tellFileOrderChanged(change: AIOutputChanged): Promise { + let tabs: chrome.tabs.Tab[]; + try { + tabs = await chrome.tabs.query({ url: 'https://github.com/*' }); + } catch { + return; + } + const message: FileOrderChangedMessage = { type: 'crs-file-order-changed' }; + for (const tab of tabs) { + const pr = tab.url ? parsePullUrl(tab.url) : null; + if (tab.id === undefined || !pr || !isChangeFor(change, pr)) continue; + chrome.tabs.sendMessage(tab.id, message).catch(() => { + // No content script there (yet): it asks when it loads. + }); + } +} + // --- The chips a PR page draws ------------------------------------------------ /** diff --git a/chrome_extension/src/content.ts b/chrome_extension/src/content.ts index cb341d24..bd3fa7b4 100644 --- a/chrome_extension/src/content.ts +++ b/chrome_extension/src/content.ts @@ -192,11 +192,17 @@ function onRuntimeMessage( sendResponse: (response: unknown) => void ): boolean { if (sender.id !== chrome.runtime.id) return false; - if ((message as { type?: unknown } | null)?.type === 'crs-toggle') { + const type = (message as { type?: unknown } | null)?.type; + if (type === 'crs-toggle') { toggle(); // Answer so the service worker's sendMessage resolves rather than // treating the tab as having no content script. sendResponse({ ok: true }); + } else if (type === 'crs-file-order-changed') { + // A new order landed for this tab's PR: forget the one held and ask. + order = null; + void refreshOrder(); + sendResponse({ ok: true }); } return false; } @@ -357,15 +363,12 @@ function scheduleRefresh(): void { /** * The order the server holds for the page's PR: `files` once it has one, null - * while it has none, and `asked` — when it was last asked for — so a page that - * found none can look again. + * while it has none. The service worker says when a new one lands + * (`crs-file-order-changed`), and it is asked for again then. */ -let order: { pr: PullRef; files: string[] | null; asked: number } | null = null; +let order: { pr: PullRef; files: string[] | null } | null = null; let orderSeq = 0; -/** How long a page that found no order waits before asking again. */ -const ORDER_RETRY_MS = 5000; - /** Asks the service worker for this page's PR's stored file order. */ async function requestOrder(): Promise { try { @@ -390,20 +393,17 @@ async function refreshOrder(): Promise { if (!pr) return; if (order && samePull(order.pr, pr)) { - if (order.files) { - applyFileOrder(document, order.files); - return; - } - // Nothing stored yet. The server works an order out when the PR is - // opened — the panel may be doing that in this tab right now — so look - // again now and then rather than leave the page unordered for good. - if (Date.now() - order.asked < ORDER_RETRY_MS) return; + // Nothing stored yet stays that way until the server works an order + // out — when the PR is opened, as the panel may be doing in this tab + // right now — and the service worker says so. + if (order.files) applyFileOrder(document, order.files); + return; } const seq = ++orderSeq; const reply = await requestOrder(); if (seq !== orderSeq || !reply?.pr || !samePull(reply.pr, pr)) return; - order = { pr, files: reply.files, asked: Date.now() }; + order = { pr, files: reply.files }; if (reply.files) applyFileOrder(document, reply.files); } diff --git a/chrome_extension/src/native_client.test.ts b/chrome_extension/src/native_client.test.ts index 79e28c19..96605eb0 100644 --- a/chrome_extension/src/native_client.test.ts +++ b/chrome_extension/src/native_client.test.ts @@ -1,5 +1,6 @@ import { afterAll, describe, expect, spyOn, test } from 'bun:test'; -import { NativeClient, type NativePort } from './native_client'; +import { NativeClient, SUBSCRIBE_PARAMS, type NativePort } from './native_client'; +import type { JsonRpcNotification } from './native_protocol'; /** A native port the test drives by hand. */ class FakePort implements NativePort { @@ -227,3 +228,97 @@ describe('NativeClient', () => { expect(!r.ok && r.error.kind).toBe('timeout'); }); }); + +describe('NativeClient notifications', () => { + function subscribing() { + const ports: FakePort[] = []; + const heard: JsonRpcNotification[] = []; + let subscribed = 0; + const client = new NativeClient({ + connect: () => { + const port = new FakePort(); + ports.push(port); + return port; + }, + lastError: () => undefined, + onNotification: n => heard.push(n), + onSubscribed: () => subscribed++, + }); + return { client, ports, heard, subscribed: () => subscribed }; + } + + const CHANGE = { + method: 'Server.AIOutputChanged', + params: [{ owner: 'acme', repo: 'widgets', number: 42, feature: 'x', status: 'pending' }], + id: null, + }; + + test('a new server is subscribed before the call that started it', async () => { + const { client, ports, subscribed } = subscribing(); + const reply = client.call('RPCHandler.GetPR', { Number: 5 }); + expect(ports[0].sent).toEqual([ + { method: 'RPCHandler.Subscribe', params: [SUBSCRIBE_PARAMS], id: 1 }, + { method: 'RPCHandler.GetPR', params: [{ Number: 5 }], id: 2 }, + ]); + + ports[0].receive({ id: 1, result: { seq: 0 }, error: null }); + ports[0].receive({ id: 2, result: 'pr', error: null }); + expect(await reply).toEqual({ ok: true, result: 'pr' }); + await Promise.resolve(); + expect(subscribed()).toBe(1); + + // One subscription per server: the port is reused. + void client.call('RPCHandler.Hello', {}); + expect(ports).toHaveLength(1); + expect(ports[0].sent).toHaveLength(3); + }); + + test('hands notifications on, whole or chunked, between replies', async () => { + const { client, ports, heard } = subscribing(); + const reply = client.call('RPCHandler.Hello', {}); + ports[0].receive(CHANGE); + const text = JSON.stringify(CHANGE); + ports[0].receive({ crs_chunk: { stream: 1, seq: 0, total: 2, data: text.slice(0, 20) } }); + ports[0].receive({ crs_chunk: { stream: 1, seq: 1, total: 2, data: text.slice(20) } }); + ports[0].receive({ id: 2, result: 'hi', error: null }); + + expect(await reply).toEqual({ ok: true, result: 'hi' }); + expect(heard).toEqual([ + { method: CHANGE.method, params: CHANGE.params }, + { method: CHANGE.method, params: CHANGE.params }, + ]); + }); + + test('subscribes again once the server is replaced', async () => { + const { client, ports, subscribed } = subscribing(); + void client.call('RPCHandler.Hello', {}); + ports[0].receive({ id: 1, result: {}, error: null }); + await Promise.resolve(); + await Promise.resolve(); + expect(subscribed()).toBe(1); + ports[0].disconnect(); + + void client.call('RPCHandler.Hello', {}); + expect(ports).toHaveLength(2); + expect((ports[1].sent[0] as { method: string }).method).toBe('RPCHandler.Subscribe'); + ports[1].receive({ id: (ports[1].sent[0] as { id: number }).id, result: {}, error: null }); + await Promise.resolve(); + await Promise.resolve(); + expect(subscribed()).toBe(2); + }); + + test('a server that refuses the subscription still answers calls', async () => { + const { client, ports, subscribed } = subscribing(); + const reply = client.call('RPCHandler.Hello', {}); + ports[0].receive({ id: 1, result: null, error: "rpc: can't find method" }); + ports[0].receive({ id: 2, result: 'hi', error: null }); + expect(await reply).toEqual({ ok: true, result: 'hi' }); + expect(subscribed()).toBe(0); + }); + + test('without a listener nothing is subscribed', () => { + const { client, ports } = setup(); + void client.call('RPCHandler.Hello', {}); + expect(ports[0].sent).toEqual([{ method: 'RPCHandler.Hello', params: [{}], id: 1 }]); + }); +}); diff --git a/chrome_extension/src/native_client.ts b/chrome_extension/src/native_client.ts index c7c28ce9..f675968a 100644 --- a/chrome_extension/src/native_client.ts +++ b/chrome_extension/src/native_client.ts @@ -1,7 +1,8 @@ // The service worker's connection to the native host: one port, opened on the // first call and reused, carrying JSON-RPC requests out and responses (whole -// or chunked) and host events back. The port is injected so tests can drive -// it without Chrome; background.ts passes chrome.runtime.connectNative. +// or chunked), the server's notifications and host events back. The port is +// injected so tests can drive it without Chrome; background.ts passes +// chrome.runtime.connectNative. // // One open port is one running server: the host starts `crs --server` when // Chrome connects and stops it when the port closes. An open @@ -19,6 +20,7 @@ import { type ErrorInfo, type HostFailure, type HostReady, + type JsonRpcNotification, type JsonRpcResponse, } from './native_protocol'; import type { HostStatus, RpcReply } from './types'; @@ -26,6 +28,15 @@ import type { HostStatus, RpcReply } from './types'; /** GetPR on a cache miss goes to GitHub, so be generous. */ export const DEFAULT_TIMEOUT_MS = 120_000; +export const SUBSCRIBE_METHOD = 'RPCHandler.Subscribe'; + +/** + * What a new server is asked to subscribe with. The extension follows the + * server's change events and has no use for its heartbeat, so it asks for the + * slowest one the server allows. + */ +export const SUBSCRIBE_PARAMS = { IntervalSeconds: 300 }; + /** The parts of chrome.runtime.Port the client uses. */ export interface NativePort { postMessage(message: unknown): void; @@ -42,6 +53,16 @@ export interface NativeClientOptions { */ lastError: () => string | undefined; timeoutMs?: number; + /** + * Receives each notification the server sends. When set, every new port — + * every new server — starts by subscribing, so that it sends them. + */ + onNotification?: (notification: JsonRpcNotification) => void; + /** + * Called each time a new server accepts the subscription. Anything it + * announced before then was missed, so this is when to catch up. + */ + onSubscribed?: () => void; } interface Pending { @@ -53,6 +74,8 @@ export class NativeClient { private readonly connect: () => NativePort; private readonly readLastError: () => string | undefined; private readonly timeoutMs: number; + private readonly onNotification?: (notification: JsonRpcNotification) => void; + private readonly onSubscribed?: () => void; private port: NativePort | null = null; private nextId = 1; @@ -69,6 +92,8 @@ export class NativeClient { this.connect = options.connect; this.readLastError = options.lastError; this.timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS; + this.onNotification = options.onNotification; + this.onSubscribed = options.onSubscribed; } /** @@ -136,15 +161,34 @@ export class NativeClient { port.onDisconnect.addListener(() => { if (this.port === port) this.onDisconnect(); }); + // Ahead of the call that opened the port, so the server is subscribed + // before that call can start anything it would announce. + if (this.onNotification) this.subscribe(port); return port; } + private subscribe(port: NativePort): void { + void this.call(SUBSCRIBE_METHOD, SUBSCRIBE_PARAMS).then(reply => { + if (this.port !== port) return; + if (reply.ok) { + this.onSubscribed?.(); + } else if (reply.error.kind === 'rpc') { + // A server from before notifications: views never hear of a + // change, and show what they read when they open. + console.warn('crs: the server does not send change events:', reply.error.message); + } + }); + } + private onMessage(msg: unknown): void { const classified = classifyHostMessage(msg); switch (classified.type) { case 'response': this.onResponse(classified.response); return; + case 'notification': + this.onNotification?.(classified.notification); + return; case 'chunk': this.onChunk(classified.chunk); return; @@ -192,8 +236,10 @@ export class NativeClient { const inner = classifyHostMessage(parsed); if (inner.type === 'response') { this.onResponse(inner.response); + } else if (inner.type === 'notification') { + this.onNotification?.(inner.notification); } else { - console.error('crs: a chunked message is not a JSON-RPC response'); + console.error('crs: a chunked message is not a JSON-RPC response or notification'); } } diff --git a/chrome_extension/src/native_protocol.test.ts b/chrome_extension/src/native_protocol.test.ts index d25c8f49..71bd2761 100644 --- a/chrome_extension/src/native_protocol.test.ts +++ b/chrome_extension/src/native_protocol.test.ts @@ -37,6 +37,20 @@ describe('classifyHostMessage', () => { expect(classifyHostMessage(msg)).toEqual({ type: 'response', response: msg }); }); + test('a notification', () => { + const params = [{ owner: 'acme', repo: 'widgets', number: 42, plugin: 'lint' }]; + expect( + classifyHostMessage({ method: 'Server.PluginOutputChanged', params, id: null }) + ).toEqual({ + type: 'notification', + notification: { method: 'Server.PluginOutputChanged', params }, + }); + // A request (it has an id) is not one: the server never sends those. + expect(classifyHostMessage({ method: 'Server.Heartbeat', params: [], id: 4 }).type).toBe( + 'invalid' + ); + }); + test('a chunk', () => { const chunk = { stream: 1, seq: 0, total: 2, data: '{"id"' }; expect(classifyHostMessage({ crs_chunk: chunk })).toEqual({ type: 'chunk', chunk }); diff --git a/chrome_extension/src/native_protocol.ts b/chrome_extension/src/native_protocol.ts index 7decbc3c..8e055cb6 100644 --- a/chrome_extension/src/native_protocol.ts +++ b/chrome_extension/src/native_protocol.ts @@ -6,10 +6,13 @@ // newline-delimited JSON-RPC. Each native message it sends is one of: // // - a JSON-RPC response `{id, result, error}`, forwarded verbatim; +// - a JSON-RPC notification `{method, params, id: null}`, which the server +// sends unasked once subscribed (RPCHandler.Subscribe), also verbatim; // - a `crs_chunk`: a piece of a response too large for one native message // (Chrome caps host → extension messages at 1 MB). A stream's chunks arrive // contiguously and in order, since one writer serializes the host's output; -// concatenating their `data` and JSON.parse-ing the result gives the response; +// concatenating their `data` and JSON.parse-ing the result gives the response +// (or, should one ever be that large, the notification); // - a `crs_host` event: the server started (`ready`), could not be started // (`error`), or died (`server_exited`). The host exits after the last two. @@ -73,6 +76,15 @@ export interface JsonRpcResponse { error: unknown; } +/** + * A JSON-RPC 1.0 notification: the server speaking unasked. Its data is + * `params[0]`; it has no id and wants no reply. + */ +export interface JsonRpcNotification { + method: string; + params: unknown[]; +} + /** A JSON-RPC 1.0 request; Go's jsonrpc takes params as a one-element array. */ export interface JsonRpcRequest { method: string; @@ -82,6 +94,7 @@ export interface JsonRpcRequest { export type HostMessage = | { type: 'response'; response: JsonRpcResponse } + | { type: 'notification'; notification: JsonRpcNotification } | { type: 'chunk'; chunk: Chunk } | { type: 'host'; event: HostEvent } | { type: 'invalid'; reason: string }; @@ -142,7 +155,14 @@ export function classifyHostMessage(msg: unknown): HostMessage { if (typeof msg.id === 'number' && 'result' in msg && 'error' in msg) { return { type: 'response', response: msg as unknown as JsonRpcResponse }; } - return { type: 'invalid', reason: 'neither a response, a chunk nor a host event' }; + if (typeof msg.method === 'string' && (msg.id === null || msg.id === undefined)) { + const params = Array.isArray(msg.params) ? msg.params : []; + return { type: 'notification', notification: { method: msg.method, params } }; + } + return { + type: 'invalid', + reason: 'neither a response, a notification, a chunk nor a host event', + }; } /** A chunk stream that broke the protocol; the stream is dropped. */ diff --git a/chrome_extension/src/panel/PRList.tsx b/chrome_extension/src/panel/PRList.tsx index be4719aa..4a7386f1 100644 --- a/chrome_extension/src/panel/PRList.tsx +++ b/chrome_extension/src/panel/PRList.tsx @@ -8,7 +8,7 @@ import { usePanel } from './context'; import { ErrorCard } from './ErrorCard'; import { failed, isFatal, loaded, loading, type Resource } from './resource'; import { ReviewList } from './ReviewList'; -import { useNow } from './usePolling'; +import { useNow } from './useNow'; export interface ReviewListState { list: Resource; diff --git a/chrome_extension/src/panel/PRTools.tsx b/chrome_extension/src/panel/PRTools.tsx index b361ebcf..b4cc840b 100644 --- a/chrome_extension/src/panel/PRTools.tsx +++ b/chrome_extension/src/panel/PRTools.tsx @@ -1,9 +1,10 @@ // The PR tools view's data: GetPR first (it fills the server's caches for a // PR in no list and starts the PR's automatic plugins and applied AI // features, as opening a PR in any client does), the feature and plugin -// lists alongside it, then the outputs — polled while anything runs. +// lists alongside it, then the outputs — read again whenever the server +// announces that one changed, never polled. -import { useCallback, useEffect, useRef, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import type { PullRef } from '../github_url'; import { getAIOutput, @@ -36,22 +37,20 @@ import { type ActionState, } from './actions'; import { runOutcomeNote } from './ai_utils'; -import { automaticPluginNames } from './plugin_utils'; -import { REQUEST_GRACE_MS, shouldPoll } from './poll_utils'; +import { coalesce } from './coalesce'; +import { + automaticPluginNames, + PENDING_PLUGIN, + rerunsStillAwaited, + withAwaitedReruns, +} from './plugin_utils'; import { PRToolsView, type PRToolsHandlers } from './PRToolsView'; import { failed, isFatal, loaded, loading, settled, toRpcError, type Resource } from './resource'; -import { useNow, usePolling } from './usePolling'; +import { useNow } from './useNow'; +import { usePRChanges } from './usePRChanges'; type Outputs = Resource>; -/** What a plugin reads as from the moment its rerun is accepted. */ -const PENDING_PLUGIN: PluginOutput = { - result: '', - status: 'pending', - body: { body_type: 'markdown', body_content: '' }, - annotations: null, -}; - function withEntries(prev: Outputs, entries: Record): Outputs { return { ...prev, value: { ...(prev.value ?? {}), ...entries } }; } @@ -74,9 +73,6 @@ export function PRTools({ pr, focus, listItem, extensionId, onActivity }: PRTool const [aiOutputs, setAiOutputs] = useState>(() => loading()); const [pluginOutputs, setPluginOutputs] = useState>(() => loading()); const [actions, setActions] = useState(NO_ACTIONS); - // Runs requested vs. runs whose grace period is over: polling continues - // while they differ, even before anything reads `pending`. - const [requests, setRequests] = useState({ made: 0, expired: 0 }); const [attempt, setAttempt] = useState(0); const now = useNow(); @@ -93,28 +89,59 @@ export function PRTools({ pr, focus, listItem, extensionId, onActivity }: PRTool activity.current = onActivity; }); - const markRequested = useCallback(() => { - setRequests(r => ({ ...r, made: r.made + 1 })); - }, []); - - useEffect(() => { - if (requests.made === requests.expired) return; - const made = requests.made; - const timer = setTimeout( - () => setRequests(r => ({ ...r, expired: Math.max(r.expired, made) })), - REQUEST_GRACE_MS - ); - return () => clearTimeout(timer); - }, [requests.made, requests.expired]); + // Reads of each kind of output, counted so only the latest one's reply is + // applied (replies can arrive out of order), and so a run's reply isn't + // applied over a read that started after the run did. + const aiReads = useRef(0); + const pluginReads = useRef(0); + // Plugin reruns accepted whose result hasn't been read yet. + const awaiting = useRef>(new Set()); - const fetchOutputs = useCallback(async () => { - const [ai, pl] = await Promise.allSettled([getAIOutput(pr), getPluginOutput(pr)]); - if (!alive.current) return; + const fetchAIOutputs = useCallback(async () => { + const read = ++aiReads.current; + const [ai] = await Promise.allSettled([getAIOutput(pr)]); + if (!alive.current || read !== aiReads.current) return; setAiOutputs(prev => settled(ai, prev)); + activity.current?.(); + }, [pr]); + + const fetchPluginOutputs = useCallback(async () => { + const read = ++pluginReads.current; + let [pl] = await Promise.allSettled([getPluginOutput(pr)]); + if (!alive.current || read !== pluginReads.current) return; + if (pl.status === 'fulfilled') { + awaiting.current = rerunsStillAwaited(awaiting.current, pl.value); + pl = { status: 'fulfilled', value: withAwaitedReruns(pl.value, awaiting.current) }; + } setPluginOutputs(prev => settled(pl, prev)); activity.current?.(); }, [pr]); + const fetchOutputs = useCallback(async () => { + await Promise.all([fetchAIOutputs(), fetchPluginOutputs()]); + }, [fetchAIOutputs, fetchPluginOutputs]); + + // What the server's change events ask for: one read of a kind at a time, + // however many arrive while it's in flight. + const refreshAI = useMemo(() => coalesce(fetchAIOutputs), [fetchAIOutputs]); + const refreshPlugins = useMemo(() => coalesce(fetchPluginOutputs), [fetchPluginOutputs]); + + usePRChanges(pr, change => { + switch (change.kind) { + case 'plugins': + void refreshPlugins(); + return; + case 'ai': + void refreshAI(); + return; + case 'resync': + // A new server ran nothing the old one was running. + awaiting.current = new Set(); + void refreshAI(); + void refreshPlugins(); + } + }); + const fetchPR = useCallback(async () => { try { const value = await getPR(pr); @@ -127,11 +154,10 @@ export function PRTools({ pr, focus, listItem, extensionId, onActivity }: PRTool } finally { activity.current?.(); } - // GetPR has started the automatic runs; read the outputs, and keep - // reading for a while in case those runs haven't been marked yet. - markRequested(); + // GetPR has started the automatic runs; read the outputs. Each run + // is announced as it starts and lands, and read again then. await fetchOutputs(); - }, [pr, fetchOutputs, markRequested]); + }, [pr, fetchOutputs]); const fetchFeatures = useCallback(() => { listAIFeatures().then( @@ -153,27 +179,22 @@ export function PRTools({ pr, focus, listItem, extensionId, onActivity }: PRTool fetchPlugins(); }, [fetchPR, fetchFeatures, fetchPlugins, attempt]); - usePolling( - shouldPoll({ - aiOutputs: aiOutputs.value, - pluginOutputs: pluginOutputs.value, - justRequested: requests.made > requests.expired, - }), - fetchOutputs - ); - const runFeature = useCallback( async (feature: AIFeature, force: boolean) => { const key = aiKey(feature.id); setActions(a => begin(a, key)); + const reads = aiReads.current; try { const reply = await runAIFeature(pr, feature.id, { force }); if (!alive.current) return; const output = reply.output; - if (output) setAiOutputs(prev => withEntries(prev, { [feature.id]: output })); + // A read that started since (the run's own start, announced) + // has the newer word. + if (output && reads === aiReads.current) { + setAiOutputs(prev => withEntries(prev, { [feature.id]: output })); + } if (reply.okay) { setActions(a => finish(a, key, runOutcomeNote(reply.outcome))); - markRequested(); } else { setActions(a => fail(a, key, new RpcError('rpc', reply.message || reply.outcome)) @@ -183,12 +204,13 @@ export function PRTools({ pr, focus, listItem, extensionId, onActivity }: PRTool if (alive.current) setActions(a => fail(a, key, toRpcError(e))); } }, - [pr, markRequested] + [pr] ); const runPlugins = useCallback( async (names: string[], key: string) => { setActions(a => begin(a, key)); + const reads = pluginReads.current; try { const reply = await rerunPlugins(pr, names); if (!alive.current) return; @@ -196,17 +218,21 @@ export function PRTools({ pr, focus, listItem, extensionId, onActivity }: PRTool setActions(a => fail(a, key, new RpcError('rpc', reply.message))); return; } - // The server clears the results and runs them in the background. - setPluginOutputs(prev => - withEntries(prev, Object.fromEntries(names.map(n => [n, PENDING_PLUGIN]))) - ); + // The server cleared the results and runs the plugins in the + // background, announcing each as it starts and lands. Until a + // read shows one landed, it reads as running. + for (const name of names) awaiting.current.add(name); + if (reads === pluginReads.current) { + setPluginOutputs(prev => + withEntries(prev, Object.fromEntries(names.map(n => [n, PENDING_PLUGIN]))) + ); + } setActions(a => finish(a, key)); - markRequested(); } catch (e) { if (alive.current) setActions(a => fail(a, key, toRpcError(e))); } }, - [pr, markRequested] + [pr] ); const sync = useCallback(async () => { @@ -224,14 +250,11 @@ export function PRTools({ pr, focus, listItem, extensionId, onActivity }: PRTool : 'Synced: already up to date with GitHub.' ) ); - if (reply.updated) { - markRequested(); - await fetchOutputs(); - } + if (reply.updated) await fetchOutputs(); } catch (e) { if (alive.current) setActions(a => fail(a, SYNC, toRpcError(e))); } - }, [pr, fetchOutputs, markRequested]); + }, [pr, fetchOutputs]); const handlers: PRToolsHandlers = { onRetryAll: () => { diff --git a/chrome_extension/src/panel/coalesce.test.ts b/chrome_extension/src/panel/coalesce.test.ts new file mode 100644 index 00000000..ea9c93f1 --- /dev/null +++ b/chrome_extension/src/panel/coalesce.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, test } from 'bun:test'; +import { coalesce } from './coalesce'; + +/** A run the test finishes by hand. */ +function gate() { + const releases: (() => void)[] = []; + let calls = 0; + const run = () => { + calls++; + return new Promise(resolve => releases.push(resolve)); + }; + return { run, calls: () => calls, release: () => releases.shift()?.() }; +} + +describe('coalesce', () => { + test('a burst of calls while one runs is one more run', async () => { + const g = gate(); + const refresh = coalesce(g.run); + const first = refresh(); + refresh(); + refresh(); + const last = refresh(); + expect(g.calls()).toBe(1); + + g.release(); + await Promise.resolve(); + await Promise.resolve(); + expect(g.calls()).toBe(2); + g.release(); + await first; + await last; + expect(g.calls()).toBe(2); + }); + + test('a call after the runs ended starts a new one', async () => { + const g = gate(); + const refresh = coalesce(g.run); + const one = refresh(); + g.release(); + await one; + const two = refresh(); + expect(g.calls()).toBe(2); + g.release(); + await two; + }); + + test('a failing run does not stop the next', async () => { + let calls = 0; + const refresh = coalesce(async () => { + calls++; + throw new Error('boom'); + }); + await refresh(); + await refresh(); + expect(calls).toBe(2); + }); +}); diff --git a/chrome_extension/src/panel/coalesce.ts b/chrome_extension/src/panel/coalesce.ts new file mode 100644 index 00000000..4f8fb40b --- /dev/null +++ b/chrome_extension/src/panel/coalesce.ts @@ -0,0 +1,34 @@ +/** + * Wraps `run` so its calls never overlap: a call made while a run is in + * flight asks for one more run once it ends, however many such calls there + * are. The server announces a burst of changes at once (each plugin of a run + * starting, then landing), and each wants the same read. + * + * The returned promise settles when the run it joined (or the one it asked + * for) has ended. `run` reports its own errors. + */ +export function coalesce(run: () => Promise): () => Promise { + let running: Promise | null = null; + let again = false; + return () => { + if (running) { + again = true; + return running; + } + running = (async () => { + try { + do { + again = false; + try { + await run(); + } catch { + // `run` reports its own failures; keep the loop going. + } + } while (again); + } finally { + running = null; + } + })(); + return running; + }; +} diff --git a/chrome_extension/src/panel/plugin_utils.test.ts b/chrome_extension/src/panel/plugin_utils.test.ts index 81ec5b88..0d876afd 100644 --- a/chrome_extension/src/panel/plugin_utils.test.ts +++ b/chrome_extension/src/panel/plugin_utils.test.ts @@ -2,13 +2,16 @@ import { describe, expect, test } from 'bun:test'; import type { PluginConfig, PluginOutput } from '../types'; import { automaticPluginNames, + PENDING_PLUGIN, pluginRunAction, pluginStatus, pluginStatusLabel, pluginStatusTone, + rerunsStillAwaited, resolvePluginBody, severityTone, sortAnnotations, + withAwaitedReruns, } from './plugin_utils'; function output(overrides: Partial = {}): PluginOutput { @@ -119,3 +122,26 @@ describe('automaticPluginNames', () => { expect(automaticPluginNames([plugin('style', true)])).toEqual([]); }); }); + +describe('awaited reruns', () => { + test('a rerun missing from the output reads as running', () => { + const lint = output(); + expect(withAwaitedReruns({ lint }, new Set(['summarize']))).toEqual({ + lint, + summarize: PENDING_PLUGIN, + }); + // Present (pending or landed) or not awaited: the output as read. + const outputs = { summarize: output({ status: 'pending' }) }; + expect(withAwaitedReruns(outputs, new Set(['summarize']))).toBe(outputs); + expect(withAwaitedReruns(outputs, new Set())).toBe(outputs); + }); + + test('a rerun is awaited until a result that is not pending is read', () => { + const awaiting = new Set(['summarize', 'lint', 'style']); + const still = rerunsStillAwaited(awaiting, { + summarize: output({ status: 'pending' }), + lint: output({ status: 'error' }), + }); + expect([...still].sort()).toEqual(['style', 'summarize']); + }); +}); diff --git a/chrome_extension/src/panel/plugin_utils.ts b/chrome_extension/src/panel/plugin_utils.ts index 87c112e5..8715fba0 100644 --- a/chrome_extension/src/panel/plugin_utils.ts +++ b/chrome_extension/src/panel/plugin_utils.ts @@ -70,6 +70,45 @@ export function isPluginPending(output: PluginOutput | null | undefined): boolea return pluginStatus(output) === 'pending'; } +/** What a plugin reads as from the moment its rerun is accepted. */ +export const PENDING_PLUGIN: PluginOutput = { + result: '', + status: 'pending', + body: { body_type: 'markdown', body_content: '' }, + annotations: null, +}; + +/** + * The output to show while reruns are awaited. A rerun deletes the plugin's + * stored result before the server gets round to marking it pending, so for a + * moment the plugin is missing from GetPluginOutput altogether; it reads as + * pending meanwhile rather than as never run. + */ +export function withAwaitedReruns( + outputs: Record, + awaiting: ReadonlySet +): Record { + const missing = [...awaiting].filter(name => !(name in outputs)); + if (missing.length === 0) return outputs; + const merged = { ...outputs }; + for (const name of missing) merged[name] = PENDING_PLUGIN; + return merged; +} + +/** + * The reruns still awaited once `outputs` has arrived. The server deletes a + * plugin's result before it accepts the rerun, so any result read after that + * which isn't pending is the rerun's own. + */ +export function rerunsStillAwaited( + awaiting: ReadonlySet, + outputs: Record +): Set { + return new Set( + [...awaiting].filter(name => !(name in outputs) || isPluginPending(outputs[name])) + ); +} + /** * The body to render: the parsed body when it has a known type, else the raw * stdout as markdown (what a plugin printing plain text produces). diff --git a/chrome_extension/src/panel/poll_utils.test.ts b/chrome_extension/src/panel/poll_utils.test.ts deleted file mode 100644 index a40728e0..00000000 --- a/chrome_extension/src/panel/poll_utils.test.ts +++ /dev/null @@ -1,34 +0,0 @@ -import { describe, expect, test } from 'bun:test'; -import type { AIFeatureOutput, PluginOutput } from '../types'; -import { anythingPending, shouldPoll } from './poll_utils'; - -const ai = (status: string) => ({ status }) as AIFeatureOutput; -const plugin = (status: string) => ({ status }) as PluginOutput; - -describe('polling decision', () => { - test('nothing loaded, nothing requested: no polling', () => { - expect(shouldPoll({ aiOutputs: null, pluginOutputs: null, justRequested: false })).toBe( - false - ); - }); - - test('everything settled: no polling', () => { - const aiOutputs = { a: ai('success'), b: ai('not-run'), c: ai('error') }; - const pluginOutputs = { p: plugin('success'), q: plugin('deferred') }; - expect(anythingPending(aiOutputs, pluginOutputs)).toBe(false); - expect(shouldPoll({ aiOutputs, pluginOutputs, justRequested: false })).toBe(false); - }); - - test('a pending AI feature polls', () => { - expect(anythingPending({ a: ai('success'), b: ai('pending') }, {})).toBe(true); - }); - - test('a pending plugin polls', () => { - expect(anythingPending(null, { p: plugin('pending') })).toBe(true); - expect(anythingPending(null, { p: plugin('PENDING') })).toBe(true); - }); - - test('a run just requested polls before anything reads pending', () => { - expect(shouldPoll({ aiOutputs: {}, pluginOutputs: {}, justRequested: true })).toBe(true); - }); -}); diff --git a/chrome_extension/src/panel/poll_utils.ts b/chrome_extension/src/panel/poll_utils.ts deleted file mode 100644 index 2cfb9f71..00000000 --- a/chrome_extension/src/panel/poll_utils.ts +++ /dev/null @@ -1,38 +0,0 @@ -// When the PR tools view polls GetAIOutput / GetPluginOutput. - -import type { AIFeatureOutput, PluginOutput } from '../types'; -import { isAIPending } from './ai_utils'; -import { isPluginPending } from './plugin_utils'; - -export const POLL_INTERVAL_MS = 3000; - -/** - * How long after a run is requested (or the PR opened, which starts its - * automatic plugins and applied AI features) the view polls even though - * nothing reads `pending` yet: a run is dispatched in the background, so the - * first read can come before it is marked. - */ -export const REQUEST_GRACE_MS = 10_000; - -export interface PollInputs { - aiOutputs: Record | null; - pluginOutputs: Record | null; - /** A run was requested within the last REQUEST_GRACE_MS. */ - justRequested: boolean; -} - -/** Whether any AI output or plugin is running. */ -export function anythingPending( - aiOutputs: Record | null, - pluginOutputs: Record | null -): boolean { - return ( - Object.values(aiOutputs ?? {}).some(isAIPending) || - Object.values(pluginOutputs ?? {}).some(isPluginPending) - ); -} - -/** Whether to keep polling. */ -export function shouldPoll({ aiOutputs, pluginOutputs, justRequested }: PollInputs): boolean { - return justRequested || anythingPending(aiOutputs, pluginOutputs); -} diff --git a/chrome_extension/src/panel/useNow.ts b/chrome_extension/src/panel/useNow.ts new file mode 100644 index 00000000..d9fda7c0 --- /dev/null +++ b/chrome_extension/src/panel/useNow.ts @@ -0,0 +1,11 @@ +import { useEffect, useState } from 'react'; + +/** The current time, refreshed every `intervalMs`, for relative times on screen. */ +export function useNow(intervalMs = 30_000): number { + const [now, setNow] = useState(() => Date.now()); + useEffect(() => { + const id = setInterval(() => setNow(Date.now()), intervalMs); + return () => clearInterval(id); + }, [intervalMs]); + return now; +} diff --git a/chrome_extension/src/panel/usePRChanges.ts b/chrome_extension/src/panel/usePRChanges.ts new file mode 100644 index 00000000..24be73a9 --- /dev/null +++ b/chrome_extension/src/panel/usePRChanges.ts @@ -0,0 +1,75 @@ +import { useEffect, useRef } from 'react'; +import type { PullRef } from '../github_url'; +import { changeFor, parseServerEvent, type PRChange } from '../server_events'; +import { EVENTS_PORT } from '../types'; + +/** The parts of chrome.runtime.Port the hook uses, so tests can stand in. */ +export interface EventsPort { + onMessage: { addListener(callback: (message: unknown) => void): void }; + onDisconnect: { addListener(callback: () => void): void }; + disconnect(): void; +} + +/** How soon a dropped events port is opened again. */ +export const RECONNECT_MS = 1000; + +function connectEvents(): EventsPort { + return chrome.runtime.connect({ name: EVENTS_PORT }); +} + +/** + * Calls `onChange` for each change the server announces to `pr`'s plugin + * output or AI results, relayed by the service worker over an events port — + * so the view reads its output again when it changes instead of polling for + * it. A `resync` means events may have been missed (a new server, or the + * port dropped and was opened again), and everything is to be read again. + * + * `onChange` is read when an event arrives, so a new identity each render + * doesn't reopen the port. + */ +export function usePRChanges( + pr: PullRef, + onChange: (change: PRChange) => void, + connect: () => EventsPort = connectEvents +): void { + const latest = useRef(onChange); + useEffect(() => { + latest.current = onChange; + }); + + const { owner, repo, number } = pr; + useEffect(() => { + let stopped = false; + let port: EventsPort | null = null; + let timer: ReturnType | undefined; + + const open = (reopened: boolean) => { + try { + port = connect(); + } catch { + // The extension was reloaded under this page: nothing to hear. + return; + } + port.onMessage.addListener(message => { + const event = parseServerEvent(message); + const change = event && changeFor(event, { owner, repo, number }); + if (change && !stopped) latest.current(change); + }); + // The service worker went away (an extension update, say). Open + // the port again, which wakes it, and catch up on what was missed. + port.onDisconnect.addListener(() => { + port = null; + if (stopped) return; + timer = setTimeout(() => open(true), RECONNECT_MS); + }); + if (reopened) latest.current({ kind: 'resync' }); + }; + + open(false); + return () => { + stopped = true; + clearTimeout(timer); + port?.disconnect(); + }; + }, [owner, repo, number, connect]); +} diff --git a/chrome_extension/src/panel/usePolling.ts b/chrome_extension/src/panel/usePolling.ts deleted file mode 100644 index c849d906..00000000 --- a/chrome_extension/src/panel/usePolling.ts +++ /dev/null @@ -1,49 +0,0 @@ -import { useEffect, useRef, useState } from 'react'; -import { POLL_INTERVAL_MS } from './poll_utils'; - -/** - * Calls `poll` every `intervalMs` while `active`, waiting for each call to - * settle before scheduling the next so slow replies never stack up. Stops - * when `active` turns false or the component unmounts. - */ -export function usePolling( - active: boolean, - poll: () => Promise, - intervalMs = POLL_INTERVAL_MS -): void { - const latest = useRef(poll); - useEffect(() => { - latest.current = poll; - }); - - useEffect(() => { - if (!active) return; - let stopped = false; - let timer: ReturnType | undefined; - const schedule = () => { - timer = setTimeout(async () => { - try { - await latest.current(); - } catch { - // The poll reports its own errors; keep going. - } - if (!stopped) schedule(); - }, intervalMs); - }; - schedule(); - return () => { - stopped = true; - clearTimeout(timer); - }; - }, [active, intervalMs]); -} - -/** The current time, refreshed every `intervalMs`, for relative times on screen. */ -export function useNow(intervalMs = 30_000): number { - const [now, setNow] = useState(() => Date.now()); - useEffect(() => { - const id = setInterval(() => setNow(Date.now()), intervalMs); - return () => clearInterval(id); - }, [intervalMs]); - return now; -} diff --git a/chrome_extension/src/server_events.test.ts b/chrome_extension/src/server_events.test.ts new file mode 100644 index 00000000..1b6eb19f --- /dev/null +++ b/chrome_extension/src/server_events.test.ts @@ -0,0 +1,88 @@ +import { describe, expect, test } from 'bun:test'; +import { changeFor, isChangeFor, parseServerEvent, toServerEvent } from './server_events'; + +const PR = { owner: 'acme', repo: 'widgets', number: 42 }; + +describe('toServerEvent', () => { + test('a plugin output change', () => { + expect( + toServerEvent({ + method: 'Server.PluginOutputChanged', + params: [{ ...PR, plugin: 'lint', status: 'success' }], + }) + ).toEqual({ + type: 'plugin-output-changed', + change: { ...PR, plugin: 'lint', status: 'success' }, + }); + // A rerun of them all clears every result at once. + expect( + toServerEvent({ method: 'Server.PluginOutputChanged', params: [{ ...PR }] }) + ).toEqual({ type: 'plugin-output-changed', change: { ...PR, plugin: '', status: '' } }); + }); + + test('an AI output change', () => { + expect( + toServerEvent({ + method: 'Server.AIOutputChanged', + params: [{ ...PR, feature: 'file-ordering', status: 'pending' }], + }) + ).toEqual({ + type: 'ai-output-changed', + change: { ...PR, feature: 'file-ordering', status: 'pending' }, + }); + expect(toServerEvent({ method: 'Server.AIOutputChanged', params: [PR] })).toBeNull(); + }); + + test('nothing for the heartbeat, an unknown method or a malformed change', () => { + expect(toServerEvent({ method: 'Server.Heartbeat', params: [{ seq: 1 }] })).toBeNull(); + expect(toServerEvent({ method: 'Server.Other', params: [PR] })).toBeNull(); + expect(toServerEvent({ method: 'Server.PluginOutputChanged', params: [] })).toBeNull(); + expect( + toServerEvent({ + method: 'Server.PluginOutputChanged', + params: [{ ...PR, number: '42' }], + }) + ).toBeNull(); + }); +}); + +describe('changeFor', () => { + test("routes an event about the page's PR, whatever the case of its names", () => { + const other = { ...PR, number: 43 }; + const plugins = { ...PR, owner: 'Acme', plugin: 'lint', status: 'success' }; + expect(changeFor({ type: 'plugin-output-changed', change: plugins }, PR)).toEqual({ + kind: 'plugins', + }); + expect( + changeFor( + { + type: 'ai-output-changed', + change: { ...PR, feature: 'comments-addressed', status: 'success' }, + }, + PR + ) + ).toEqual({ kind: 'ai', feature: 'comments-addressed' }); + expect(changeFor({ type: 'plugin-output-changed', change: plugins }, other)).toBeNull(); + expect(changeFor({ type: 'resync' }, other)).toEqual({ kind: 'resync' }); + }); + + test('isChangeFor needs the same number, owner and repo', () => { + expect(isChangeFor({ ...PR, repo: 'gadgets' }, PR)).toBe(false); + expect(isChangeFor({ ...PR, owner: 'other' }, PR)).toBe(false); + expect(isChangeFor({ owner: 'ACME', repo: 'Widgets', number: 42 }, PR)).toBe(true); + }); +}); + +describe('parseServerEvent', () => { + test('reads what the service worker posts, and nothing else', () => { + expect(parseServerEvent({ type: 'resync' })).toEqual({ type: 'resync' }); + const change = { ...PR, plugin: 'lint', status: 'pending' }; + expect(parseServerEvent({ type: 'plugin-output-changed', change })).toEqual({ + type: 'plugin-output-changed', + change, + }); + expect(parseServerEvent({ type: 'ai-output-changed' })).toBeNull(); + expect(parseServerEvent('resync')).toBeNull(); + expect(parseServerEvent(null)).toBeNull(); + }); +}); diff --git a/chrome_extension/src/server_events.ts b/chrome_extension/src/server_events.ts new file mode 100644 index 00000000..b97469d0 --- /dev/null +++ b/chrome_extension/src/server_events.ts @@ -0,0 +1,86 @@ +// The server's change events, as the service worker relays them to pages. +// Pure — no chrome.* — so it is unit tested directly. +// +// Once subscribed (native_client.ts), the server sends a notification each +// time a PR's plugin output or AI results change. Only those two reach pages; +// the heartbeat and any method this extension doesn't know stay in the +// service worker. + +import type { PullRef } from './github_url'; +import type { JsonRpcNotification } from './native_protocol'; +import type { AIOutputChanged, PluginOutputChanged, ServerEventMessage } from './types'; + +export const PLUGIN_OUTPUT_CHANGED = 'Server.PluginOutputChanged'; +export const AI_OUTPUT_CHANGED = 'Server.AIOutputChanged'; + +/** The page message for a notification, or null for one pages don't hear. */ +export function toServerEvent(notification: JsonRpcNotification): ServerEventMessage | null { + const data = notification.params[0]; + if (typeof data !== 'object' || data === null) return null; + const fields = data as Record; + const { owner, repo, number } = fields; + if (typeof owner !== 'string' || typeof repo !== 'string' || typeof number !== 'number') { + return null; + } + const status = typeof fields.status === 'string' ? fields.status : ''; + switch (notification.method) { + case PLUGIN_OUTPUT_CHANGED: { + const plugin = typeof fields.plugin === 'string' ? fields.plugin : ''; + const change: PluginOutputChanged = { owner, repo, number, plugin, status }; + return { type: 'plugin-output-changed', change }; + } + case AI_OUTPUT_CHANGED: { + if (typeof fields.feature !== 'string') return null; + const change: AIOutputChanged = { + owner, + repo, + number, + feature: fields.feature, + status, + }; + return { type: 'ai-output-changed', change }; + } + default: + return null; + } +} + +/** Whether `change` is about `pr`. GitHub names ignore case. */ +export function isChangeFor(change: PullRef, pr: PullRef): boolean { + return ( + change.number === pr.number && + change.owner.toLowerCase() === pr.owner.toLowerCase() && + change.repo.toLowerCase() === pr.repo.toLowerCase() + ); +} + +/** Reads one message off an events port, or null for anything else. */ +export function parseServerEvent(message: unknown): ServerEventMessage | null { + if (typeof message !== 'object' || message === null) return null; + const msg = message as Record; + if (msg.type === 'resync') return { type: 'resync' }; + if ( + (msg.type === 'plugin-output-changed' || msg.type === 'ai-output-changed') && + typeof msg.change === 'object' && + msg.change !== null + ) { + return msg as unknown as ServerEventMessage; + } + return null; +} + +/** What a page showing `pr` does with one event: read which output again, if any. */ +export type PRChange = { kind: 'plugins' } | { kind: 'ai'; feature: string } | { kind: 'resync' }; + +export function changeFor(event: ServerEventMessage, pr: PullRef): PRChange | null { + switch (event.type) { + case 'resync': + return { kind: 'resync' }; + case 'plugin-output-changed': + return isChangeFor(event.change, pr) ? { kind: 'plugins' } : null; + case 'ai-output-changed': + return isChangeFor(event.change, pr) + ? { kind: 'ai', feature: event.change.feature } + : null; + } +} diff --git a/chrome_extension/src/types.ts b/chrome_extension/src/types.ts index 01bd9a31..ded1e0a2 100644 --- a/chrome_extension/src/types.ts +++ b/chrome_extension/src/types.ts @@ -328,6 +328,57 @@ export interface FileOrderReply { files: string[] | null; } +/** + * Service worker → content script: the file order stored for the tab's PR + * changed (a file-ordering run landed), so the page should ask for it again. + */ +export interface FileOrderChangedMessage { + type: 'crs-file-order-changed'; +} + +// --- The server's change events ------------------------------------------------ + +/** + * The name of the port an extension page opens (chrome.runtime.connect) to + * hear the server's change events. The service worker answers only the + * extension's own pages, as it does for RPC. + */ +export const EVENTS_PORT = 'crs-events'; + +/** A PR's stored plugin output changed (the server's Server.PluginOutputChanged). */ +export interface PluginOutputChanged { + owner: string; + repo: string; + number: number; + /** The plugin whose result changed; empty when a rerun cleared them all. */ + plugin: string; + /** Its status now; empty when it was cleared for a rerun. */ + status: string; +} + +/** An AI feature's output for a PR changed (the server's Server.AIOutputChanged). */ +export interface AIOutputChanged { + owner: string; + repo: string; + number: number; + feature: string; + /** `pending` as a run starts, then the stored result's status. */ + status: string; +} + +/** + * Service worker → page, over an events port. Each names what changed, never + * the output, which the page reads again. + */ +export type ServerEventMessage = + | { type: 'plugin-output-changed'; change: PluginOutputChanged } + | { type: 'ai-output-changed'; change: AIOutputChanged } + /** + * A new server is listening: whatever happened before it subscribed was + * never announced, so a page reads everything it shows again. + */ + | { type: 'resync' }; + export type ExtensionRequest = RpcRequestMessage | StatusRequestMessage | ToolsRequestMessage | FileOrderRequestMessage; diff --git a/docs/ai_features.md b/docs/ai_features.md index d90e50e6..4689060b 100644 --- a/docs/ai_features.md +++ b/docs/ai_features.md @@ -670,7 +670,8 @@ The `report` is `{"rating": "easy" | "medium" | "hard"}`. tab the content script also reorders GitHub's own diffs and file tree to match that order. Opening the view calls `GetPR`, which requests the applied features and runs the `Automatic` ones as any client - does, and the view polls `GetAIOutput` while anything is pending. + does, and the view reads `GetAIOutput` again as the server announces each + run starting and landing, without polling. The web and Emacs clients run a feature on open when it never ran or went stale; the extension runs one only when asked. diff --git a/docs/clients.md b/docs/clients.md index 5196ba21..7c66b488 100644 --- a/docs/clients.md +++ b/docs/clients.md @@ -230,7 +230,7 @@ An extension can't start processes, so the extension starts the server through a Press the toolbar button or `Alt+Shift+R`: -- **On a pull request**, a modal opens over the page on that PR's tools: a summary with the review-ease rating and **Sync**, a card per enabled AI feature with a report (the [change diagram](ai_features.md#change-diagram) drawn inline), a card for [file-ordering](ai_features.md#file-ordering) listing the suggested order, and a card per plugin with **Re-run all**. Results are polled while anything runs. Annotations link to their line in GitHub's Files changed tab. +- **On a pull request**, a modal opens over the page on that PR's tools: a summary with the review-ease rating and **Sync**, a card per enabled AI feature with a report (the [change diagram](ai_features.md#change-diagram) drawn inline), a card for [file-ordering](ai_features.md#file-ordering) listing the suggested order, and a card per plugin with **Re-run all**. Results are read again as the server announces each run starting and landing; nothing is polled. Annotations link to their line in GitHub's Files changed tab. - **Anywhere else on github.com**, the modal opens on the review list, grouped by section, with a filter. Clicking a PR takes the tab to it on GitHub; its **Tools** button opens the PR's tools in the modal instead, and **Back** returns to the list. - **Off GitHub**, the same panel opens in a popup window, where PR links open in a new tab. diff --git a/docs/plugins.md b/docs/plugins.md index f43fa546..ff16a63f 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -245,7 +245,7 @@ The [Chrome extension](clients.md#chrome-extension) shows a card per configured Annotations are listed beneath the body, sorted by file and line. The extension doesn't render the diff, so each one links to its line in GitHub's own Files changed tab instead (`https://github.com///pull//files#diff-R`, the anchor GitHub gives a line on the head side). -**Run**, on a plugin that hasn't run or is deferred, and **Re-run**, on one that has, both call `RerunPlugins` naming that plugin alone, so a deferred plugin runs too. **Re-run all** calls it naming every plugin that runs automatically, rather than with no names, which would also clear the deferred plugins' results: an on-demand result someone asked for is kept. The view polls `GetPluginOutput` until nothing reads `pending`. +**Run**, on a plugin that hasn't run or is deferred, and **Re-run**, on one that has, both call `RerunPlugins` naming that plugin alone, so a deferred plugin runs too. **Re-run all** calls it naming every plugin that runs automatically, rather than with no names, which would also clear the deferred plugins' results: an on-demand result someone asked for is kept. The view reads `GetPluginOutput` again each time the server announces a change to the PR's plugin output ([`Server.PluginOutputChanged`](protocol.md#serverpluginoutputchanged)); it never polls. ## On-Demand Plugins From df832896332de1ad917e5c76bbea413f1998b4c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 26 Oct 2026 13:32:23 +0000 Subject: [PATCH 3/5] Emacs client: follow the server's plugin and AI runs instead of polling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit crs-start-server now subscribes to the server's notifications (with the slowest heartbeat, which the client doesn't use), and the process filter hands a notification — a line with a `method` and no id — to crs-server-notification-functions instead of the reply table. An open plugin output buffer reads its output again when the server announces a change to its PR's plugins, one read at a time with a burst coalesced into one more, keeping point; the refresh now targets its own buffer rather than whichever one was current when the reply came back. A run asked for with crs-rerun-plugin or crs-run-on-demand-plugin is announced in the echo area when it lands, instead of the docstrings telling the user to poll by hand. AI report buffers and the change diagram no longer poll on a timer: they fetch again when Server.AIOutputChanged names their feature and PR. The poll timer, its kill-buffer cleanup and crs-ai-poll-interval / crs-ai-poll-timeout go. A new server's subscription has every open plugin and AI buffer read its output again, since a run the old one had in flight would never be announced. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011prZsf8nHYe7e2Ke1rrq6z --- client.el/crs-ai.el | 93 +++++++++-------- client.el/crs-diagram.el | 9 +- client.el/crs-plugins.el | 146 ++++++++++++++++++++------ client.el/crs-rpc.el | 62 +++++++++-- client.el/crs-tests.el | 220 +++++++++++++++++++++++++++++++++------ client.el/crs-vars.el | 42 +++++--- docs/ai_features.md | 3 +- docs/building_clients.md | 2 +- docs/clients.md | 12 ++- 9 files changed, 455 insertions(+), 134 deletions(-) diff --git a/client.el/crs-ai.el b/client.el/crs-ai.el index 8ff33092..a094bfd7 100644 --- a/client.el/crs-ai.el +++ b/client.el/crs-ai.el @@ -10,8 +10,10 @@ ;; ;; A run happens in the background on the server. Opening a feature's buffer ;; asks for one when the feature has never run for the PR or its result is -;; stale (the server answers from its cache when nothing changed), and the -;; buffer then polls while the output reads "pending". +;; stale (the server answers from its cache when nothing changed). The +;; server announces every run of a feature as it starts and as it lands +;; (Server.AIOutputChanged), whoever started it, and each open buffer showing +;; that feature for the PR reads its output again then: nothing polls. ;; ;; The body the server sends is a complete markdown report, so this client ;; renders it as-is; a feature without a typed report also gets its line @@ -106,12 +108,6 @@ BUFFER's `crs--ai-insert-function' inserts it, by default as a report." (goto-char (min pos (point-max)))) (setq crs--ai-output output)))) -(defun crs--ai-cancel-poll () - "Stop polling for the current AI output buffer." - (when (timerp crs--ai-poll-timer) - (cancel-timer crs--ai-poll-timer)) - (setq crs--ai-poll-timer nil)) - (defun crs--ai-args (&optional force) "RPC arguments naming the current buffer's PR and feature. FORCE, when non-nil, adds Force for RunAIFeature." @@ -128,31 +124,6 @@ FORCE, when non-nil, adds Force for RunAIFeature." ((stringp err) err) (t (format "%s" (or (cdr (assq 'message err)) err)))))) -(defun crs--ai-continue (buffer output) - "Keep polling BUFFER while OUTPUT is pending, within `crs-ai-poll-timeout'." - (when (buffer-live-p buffer) - (with-current-buffer buffer - (crs--ai-cancel-poll) - (when (crs--ai-pending-p output) - (unless crs--ai-poll-started - (setq crs--ai-poll-started (float-time))) - (if (> (- (float-time) crs--ai-poll-started) crs-ai-poll-timeout) - (progn - (setq crs--ai-poll-started nil) - (message "%s is still running; press r later to refresh." - (cdr (assq 'name crs--ai-feature)))) - (setq crs--ai-poll-timer - (run-with-timer crs-ai-poll-interval nil #'crs--ai-poll buffer)))) - (unless (crs--ai-pending-p output) - (setq crs--ai-poll-started nil))))) - -(defun crs--ai-poll (buffer) - "Fetch the output shown in BUFFER again, if BUFFER still exists." - (when (buffer-live-p buffer) - (with-current-buffer buffer - (setq crs--ai-poll-timer nil) - (crs--ai-fetch buffer nil)))) - (defun crs--ai-fetch (buffer run-if-needed) "Fetch and render the output for BUFFER's PR and feature. With RUN-IF-NEEDED, start a run when there is no current result." @@ -170,13 +141,13 @@ With RUN-IF-NEEDED, start a run when there is no current result." ((null output) (message "The server returned no output for %s." feature-id)) (t (crs--ai-render buffer output) - (if (and run-if-needed (crs--ai-should-run-p output)) - (crs--ai-run buffer nil) - (crs--ai-continue buffer output))))))))))) + (when (and run-if-needed (crs--ai-should-run-p output)) + (crs--ai-run buffer nil))))))))))) (defun crs--ai-run (buffer force) - "Ask the server to run BUFFER's feature for its PR, then poll. -With FORCE, run even when the stored result covers the PR's inputs." + "Ask the server to run BUFFER's feature for its PR. +With FORCE, run even when the stored result covers the PR's inputs. The +server announces the run landing, and BUFFER reads it then." (when (buffer-live-p buffer) (with-current-buffer buffer (crs--send-request @@ -191,9 +162,43 @@ With FORCE, run even when the stored result covers the PR's inputs." (when output (crs--ai-render buffer output)) (unless (eq (cdr (assq 'okay result)) t) - (message "%s" (cdr (assq 'message result)))) - (when output - (crs--ai-continue buffer output)))))))))) + (message "%s" (cdr (assq 'message result)))))))))))) + +(defun crs--ai-buffers (&optional owner repo number feature-id) + "Live buffers showing an AI feature, or only FEATURE-ID's for OWNER/REPO #NUMBER. +That is report buffers and the change diagram's alike." + (seq-filter (lambda (buffer) + (let ((feature (buffer-local-value 'crs--ai-feature buffer))) + (and feature + (or (null number) + (and (equal (cdr (assq 'id feature)) feature-id) + (with-current-buffer buffer + (crs--server-same-pr-p + (list (cons 'owner owner) (cons 'repo repo) + (cons 'number number)) + crs--ai-owner crs--ai-repo crs--ai-number))))))) + (buffer-list))) + +(defun crs--ai-on-notification (method data) + "Follow a Server.AIOutputChanged notification, METHOD with DATA. +Each open buffer showing the feature for the PR reads its output again." + (when (equal method "Server.AIOutputChanged") + (let ((owner (cdr (assq 'owner data))) + (repo (cdr (assq 'repo data))) + (number (cdr (assq 'number data))) + (feature (cdr (assq 'feature data)))) + (when (and (stringp owner) (stringp repo) (integerp number) (stringp feature)) + (dolist (buffer (crs--ai-buffers owner repo number feature)) + (crs--ai-fetch buffer nil)))))) + +(defun crs--ai-on-subscribed () + "Read every open AI buffer's output again, for a new server. +A run the old one had in flight is gone with it, so none would land." + (dolist (buffer (crs--ai-buffers)) + (crs--ai-fetch buffer nil))) + +(add-hook 'crs-server-notification-functions #'crs--ai-on-notification) +(add-hook 'crs-server-subscribed-hook #'crs--ai-on-subscribed) (defun crs--ai-report-features (features) "The FEATURES, as ListAIFeatures lists them, that have a report to open. @@ -269,7 +274,8 @@ report buffer." (defun crs-get-ai-output () "Show an AI feature's report for the PR in the current review buffer. Starts a run when the feature has never run for the PR or its report is -stale, and keeps the buffer updated while the run is pending." +stale. The buffer shows each run of the feature for the PR as it starts +and lands." (interactive) (let* ((info (crs--get-current-review-info)) (owner (nth 0 info)) @@ -327,8 +333,7 @@ already covers the PR." "Major mode for an AI feature's report on a PR. Inherits from `markdown-mode' and is read-only. \\{crs-ai-output-mode-map}" - (setq buffer-read-only t) - (add-hook 'kill-buffer-hook #'crs--ai-cancel-poll nil t)) + (setq buffer-read-only t)) (provide 'crs-ai) ;;; crs-ai.el ends here diff --git a/client.el/crs-diagram.el b/client.el/crs-diagram.el index 4f145db3..167e1e53 100644 --- a/client.el/crs-diagram.el +++ b/client.el/crs-diagram.el @@ -11,8 +11,8 @@ ;; ;; The buffer is an AI output buffer like crs-ai.el's, and shares its ;; machinery: opening it asks for a run when the diagram was never drawn -;; for the PR or the PR has changed since, it polls while the run is -;; pending, and `r', `R' and `q' refresh, re-run and quit. Unlike a report +;; for the PR or the PR has changed since, it redraws as the server +;; announces the run landing, and `r', `R' and `q' refresh, re-run and quit. Unlike a report ;; buffer, it holds nothing but the diagram, so mermaid-mode sees valid ;; Mermaid: the status goes in the header line, and when there is no ;; diagram, a Mermaid comment says why. @@ -110,8 +110,7 @@ The buffer is read-only and holds the diagram's Mermaid source, in (fundamental-mode) (message "Install mermaid-mode to highlight and preview the diagram.")) (setq buffer-read-only t) - (crs-diagram-view-mode 1) - (add-hook 'kill-buffer-hook #'crs--ai-cancel-poll nil t)) + (crs-diagram-view-mode 1)) (setq crs--ai-owner owner crs--ai-repo repo crs--ai-number number @@ -157,7 +156,7 @@ or the one on the current line of the reviews list; anywhere else it is read from the minibuffer. Draws the diagram when it was never drawn for the PR or the PR has -changed since, and keeps the buffer updated while that runs. In the +changed since, and redraws it as the server announces the run landing. In the buffer, \\\\[crs-ai-refresh] refreshes, \\[crs-ai-rerun] draws it afresh and \\[crs-quit-ai-output] quits." (interactive (crs--diagram-read-pr)) (crs--ai-fetch (crs--diagram-open owner repo number) t)) diff --git a/client.el/crs-plugins.el b/client.el/crs-plugins.el index 951f009d..7d8b9b36 100644 --- a/client.el/crs-plugins.el +++ b/client.el/crs-plugins.el @@ -3,6 +3,13 @@ ;;; Commentary: ;; Plugin-output buffers and on-demand plugin execution. +;; +;; Plugins run in the background on the server, which announces each change +;; to a PR's plugin output (Server.PluginOutputChanged): a plugin marked +;; pending, a result landing, a rerun clearing one. An open plugin output +;; buffer for the PR reads its output again on each, so it follows a run +;; without being refreshed by hand, and a run asked for from here is +;; announced in the echo area when it lands. ;;; Code: @@ -54,38 +61,113 @@ listed beneath the body sorted by file and line." (replace-regexp-in-string "\n" "\n " content)))))) (insert "\n"))) +(defun crs--refresh-plugin-buffer (buffer &optional quiet) + "Fetch the plugin output shown in BUFFER again and redraw it. +With QUIET, as for a change the server announced, say nothing and keep +point where the reader left it. One fetch runs at a time per buffer: +one asked for meanwhile runs once it lands." + (when (buffer-live-p buffer) + (with-current-buffer buffer + (if crs--plugin-refreshing + (setq crs--plugin-refreshing 'again) + (setq crs--plugin-refreshing t) + (let ((owner crs--plugin-owner) + (repo crs--plugin-repo) + (number crs--plugin-number)) + (unless quiet + (message "Refreshing plugin output for %s/%s #%d..." owner repo number)) + (crs--send-request + "RPCHandler.GetPluginOutput" + (vector (list (cons 'Owner owner) + (cons 'Repo repo) + (cons 'Number number))) + (lambda (result) + (when (buffer-live-p buffer) + (with-current-buffer buffer + (let ((again (eq crs--plugin-refreshing 'again))) + (setq crs--plugin-refreshing nil) + (unless (cdr (assq 'error result)) + (crs--render-plugin-buffer (cdr (assq 'output result)) quiet) + (unless quiet (message "Plugin output refreshed."))) + (when again (crs--refresh-plugin-buffer buffer t)))))))))))) + +(defun crs--render-plugin-buffer (output keep-point) + "Redraw the current plugin output buffer from OUTPUT, a GetPluginOutput map. +A buffer showing one plugin shows only that one. With KEEP-POINT, point +stays where it was rather than going back to the top." + (let ((inhibit-read-only t) + (pos (point)) + (target-plugin crs--plugin-name)) + (erase-buffer) + (setq crs--plugin-output-map (make-hash-table :test 'equal)) + (if (null output) + (insert "No plugin output available.\n") + (dolist (plugin-entry (append output nil)) + (let* ((name (symbol-name (car plugin-entry))) + (data (cdr plugin-entry))) + (puthash name data crs--plugin-output-map) + (when (or (null target-plugin) (string= name target-plugin)) + (crs--insert-plugin-output-entry name data))))) + (goto-char (if keep-point (min pos (point-max)) (point-min))))) + (defun crs-refresh-plugin-output () "Refresh the plugin output in the current buffer." (interactive) (unless (and crs--plugin-owner crs--plugin-repo crs--plugin-number) (error "Not in a plugin output buffer or missing PR context")) - (let ((owner crs--plugin-owner) - (repo crs--plugin-repo) - (number crs--plugin-number) - (target-plugin crs--plugin-name)) - (message "Refreshing plugin output for %s/%s #%d..." owner repo number) - (crs--send-request - "RPCHandler.GetPluginOutput" - (vector (list (cons 'Owner owner) - (cons 'Repo repo) - (cons 'Number number))) - (lambda (result) - (let ((output (cdr (assq 'output result))) - (buffer (current-buffer))) - (with-current-buffer buffer - (let ((inhibit-read-only t)) - (erase-buffer) - (setq crs--plugin-output-map (make-hash-table :test 'equal)) - (if (null output) - (insert "No plugin output available.\n") - (dolist (plugin-entry (append output nil)) - (let* ((name (symbol-name (car plugin-entry))) - (data (cdr plugin-entry))) - (puthash name data crs--plugin-output-map) - (when (or (null target-plugin) (string= name target-plugin)) - (crs--insert-plugin-output-entry name data))))) - (goto-char (point-min)))) - (message "Plugin output refreshed.")))))) + (crs--refresh-plugin-buffer (current-buffer))) + +(defun crs--plugin-buffers (&optional owner repo number) + "Live plugin output buffers, or only those showing OWNER/REPO #NUMBER." + (seq-filter (lambda (buffer) + (with-current-buffer buffer + (and (derived-mode-p 'crs-plugin-output-mode) + crs--plugin-number + (or (null number) + (crs--server-same-pr-p + (list (cons 'owner owner) (cons 'repo repo) + (cons 'number number)) + crs--plugin-owner crs--plugin-repo crs--plugin-number))))) + (buffer-list))) + +(defun crs--plugin-run-key (owner repo number plugin) + "The `crs--plugin-runs-requested' key of PLUGIN's run on OWNER/REPO #NUMBER." + (format "%s/%s#%d:%s" (downcase owner) (downcase repo) number plugin)) + +(defun crs--plugin-on-notification (method data) + "Follow a Server.PluginOutputChanged notification, METHOD with DATA. +Each open plugin output buffer for the PR reads its output again, and a +run asked for from here is announced once it lands." + (when (equal method "Server.PluginOutputChanged") + (let ((owner (cdr (assq 'owner data))) + (repo (cdr (assq 'repo data))) + (number (cdr (assq 'number data))) + (plugin (cdr (assq 'plugin data))) + (status (cdr (assq 'status data)))) + (when (and (stringp owner) (stringp repo) (integerp number)) + (dolist (buffer (crs--plugin-buffers owner repo number)) + (crs--refresh-plugin-buffer buffer t)) + (when (and (stringp plugin) (member status '("success" "error"))) + (let ((key (crs--plugin-run-key owner repo number plugin))) + (when (gethash key crs--plugin-runs-requested) + (remhash key crs--plugin-runs-requested) + (message "Plugin '%s' %s for %s/%s #%d%s" + plugin (if (equal status "success") "finished" "failed") + owner repo number + (if (crs--plugin-buffers owner repo number) + "" + (substitute-command-keys + "; \\\\[crs-get-plugin-output] in its review shows it")))))))))) + +(defun crs--plugin-on-subscribed () + "Read every open plugin output buffer's output again, for a new server. +Runs asked for from the old one are gone with it." + (clrhash crs--plugin-runs-requested) + (dolist (buffer (crs--plugin-buffers)) + (crs--refresh-plugin-buffer buffer t))) + +(add-hook 'crs-server-notification-functions #'crs--plugin-on-notification) +(add-hook 'crs-server-subscribed-hook #'crs--plugin-on-subscribed) (defun crs-quit-plugin-output () @@ -241,7 +323,9 @@ Uses cached data from the general plugin output buffer if available." (defun crs-run-on-demand-plugin () "Run an on-demand plugin for the current PR. Presents a selection of plugins marked as OnlyOnDemand and triggers -async execution via RerunPlugins. Poll with \\[crs-get-plugin-output] to see results." +async execution via RerunPlugins. The echo area says when it lands, and +an open plugin output buffer for the PR (\\[crs-get-plugin-output]) +shows the result as it does." (interactive) (if (null crs-plugins-full) (progn @@ -259,6 +343,7 @@ async execution via RerunPlugins. Poll with \\[crs-get-plugin-output] to see re (repo (nth 1 info)) (number (nth 2 info))) (message "Running on-demand plugin '%s' for %s/%s #%d..." plugin owner repo number) + (puthash (crs--plugin-run-key owner repo number plugin) t crs--plugin-runs-requested) (crs--send-request "RPCHandler.RerunPlugins" (vector (list (cons 'Owner owner) @@ -272,7 +357,9 @@ async execution via RerunPlugins. Poll with \\[crs-get-plugin-output] to see re (defun crs-rerun-plugin () "Rerun any plugin for the current PR. Presents a selection of all configured plugins and triggers -async execution via RerunPlugins. Poll with \\[crs-get-plugin-output] to see results." +async execution via RerunPlugins. The echo area says when it lands, and +an open plugin output buffer for the PR (\\[crs-get-plugin-output]) +shows the result as it does." (interactive) (if (null crs-plugins-full) (progn @@ -287,6 +374,7 @@ async execution via RerunPlugins. Poll with \\[crs-get-plugin-output] to see re (repo (nth 1 info)) (number (nth 2 info))) (message "Rerunning plugin '%s' for %s/%s #%d..." plugin owner repo number) + (puthash (crs--plugin-run-key owner repo number plugin) t crs--plugin-runs-requested) (crs--send-request "RPCHandler.RerunPlugins" (vector (list (cons 'Owner owner) diff --git a/client.el/crs-rpc.el b/client.el/crs-rpc.el index ee133647..0177f844 100644 --- a/client.el/crs-rpc.el +++ b/client.el/crs-rpc.el @@ -39,12 +39,49 @@ Returns the process handle." ;; Check if process is still alive after startup (unless (process-live-p crs--process) (let ((stderr-content (with-current-buffer stderr-buffer (buffer-string)))) - (error "Server process died immediately. Stderr: %s" stderr-content))))) + (error "Server process died immediately. Stderr: %s" stderr-content))) + + ;; Hear about plugin and AI runs as they start and land. + (crs--subscribe))) (message "Started crs JSON-RPC server") (crs-list-plugins) crs--process)) +(defun crs--subscribe () + "Ask the server for its notifications. +Runs `crs-server-subscribed-hook' once it has accepted. A server from +before notifications refuses, and buffers then show what they read when +they were opened or refreshed by hand." + (crs--send-request + "RPCHandler.Subscribe" + (vector (list (cons 'IntervalSeconds crs--heartbeat-seconds))) + (lambda (result) + (unless (cdr (assq 'error result)) + (run-hooks 'crs-server-subscribed-hook))))) + +(defun crs--server-same-pr-p (data owner repo number) + "Non-nil when notification DATA names PR OWNER/REPO #NUMBER. +GitHub names ignore case." + (and (equal (cdr (assq 'number data)) number) + (stringp owner) (stringp repo) + (equal (downcase (or (cdr (assq 'owner data)) "")) (downcase owner)) + (equal (downcase (or (cdr (assq 'repo data)) "")) (downcase repo)))) + +(defun crs--handle-notification (method params) + "Hand the server's notification METHOD to `crs-server-notification-functions'. +PARAMS is its params array; each function gets its one element. One +that fails is reported, and the rest still run." + (let ((data (and (vectorp params) (> (length params) 0) (aref params 0)))) + (run-hook-wrapped + 'crs-server-notification-functions + (lambda (fn) + (condition-case err + (funcall fn method data) + (error (message "crs: handling %s failed: %s" + method (error-message-string err)))) + nil)))) + ;;;###autoload (defun crs-shutdown-server () "Stop the crs JSON-RPC server process if it is running." @@ -110,25 +147,32 @@ The keyword arguments reproduce `json-read-from-string' semantics exactly (json-read-from-string string))) (defun crs--handle-response (response-line) - "Handle a single JSON-RPC response line." + "Handle a single JSON-RPC line: a response, or a notification. +A notification names a `method' and has no id; it goes to +`crs--handle-notification'." (when crs-debug (message "DEBUG crs response: %s" (truncate-string-to-width response-line crs-debug-max-length nil nil t))) (condition-case err (let ((response (crs--parse-json response-line))) (let ((id (cdr (assq 'id response))) + (method (cdr (assq 'method response))) (result (cdr (assq 'result response))) (error (cdr (assq 'error response)))) - (if error - (let ((callback (gethash id crs--pending-requests))) - (message "JSON-RPC Error: %s" (if (stringp error) error (cdr (assq 'message error)))) - (when callback - (remhash id crs--pending-requests) - (funcall callback `((error . ,error))))) + (cond + ((and (stringp method) (null id)) + (crs--handle-notification method (cdr (assq 'params response)))) + (error + (let ((callback (gethash id crs--pending-requests))) + (message "JSON-RPC Error: %s" (if (stringp error) error (cdr (assq 'message error)))) + (when callback + (remhash id crs--pending-requests) + (funcall callback `((error . ,error)))))) + (t (let ((callback (gethash id crs--pending-requests))) (when callback (remhash id crs--pending-requests) - (funcall callback result)))))) + (funcall callback result))))))) (error (message "Error parsing JSON-RPC response: %s" err)))) diff --git a/client.el/crs-tests.el b/client.el/crs-tests.el index b48e6a8a..7bb78529 100644 --- a/client.el/crs-tests.el +++ b/client.el/crs-tests.el @@ -66,6 +66,9 @@ crs--annotation-comment-body crs--annotation-choice-label crs--select-annotation crs--insert-plugin-output-entry + crs--handle-notification crs--subscribe crs--server-same-pr-p + crs--refresh-plugin-buffer crs--plugin-on-notification + crs--ai-on-notification crs--ai-buffers crs--insert-ai-output crs--ai-should-run-p crs--ai-pending-p crs--ai-buffer-name crs--ai-choose-feature crs--ai-report-features crs--ai-feature-buffer @@ -83,8 +86,10 @@ crs--buffer-images crs--comment-owner crs--comment-filename crs--comment-position crs--plugin-owner crs--plugin-name crs--plugin-output-map + crs--plugin-refreshing crs--plugin-runs-requested + crs-server-notification-functions crs-server-subscribed-hook crs-ai-features crs--ai-owner crs--ai-feature crs--ai-output - crs--ai-poll-timer crs--ai-insert-function + crs--ai-insert-function crs-change-diagram-feature-id crs-diff-filters crs--buffer-diff-filter crs--buffer-full-diff crs--buffer-filtered-diff)) @@ -702,21 +707,12 @@ OVERRIDES is a plist of field symbols and values." (should (eq (lookup-key crs-ai-output-mode-map (kbd "R")) #'crs-ai-rerun)) (should (eq (lookup-key crs-ai-output-mode-map (kbd "q")) #'crs-quit-ai-output))) -(ert-deftest crs-test-ai-killing-the-buffer-stops-polling () - "A pending poll timer does not outlive its buffer." - (let ((buffer (generate-new-buffer "crs-ai-test"))) - (with-current-buffer buffer - (crs-ai-output-mode) - (setq crs--ai-poll-timer (run-with-timer 3600 nil #'ignore))) - (let ((timer (buffer-local-value 'crs--ai-poll-timer buffer))) - (kill-buffer buffer) - (should-not (memq timer timer-list))))) - (defmacro crs-test--with-fake-rpc (replies &rest body) "Run BODY with RPCs answered from REPLIES and timers recorded, not run. -REPLIES maps a method name to the result its callback receives. Binds +REPLIES maps a method name to the result its callback receives; it is +evaluated for each call, so a variable there can change mid-test. Binds `calls' to the (METHOD . PARAMS) sent, most recent first, and `timers' to -the functions scheduled." +the functions scheduled: nothing should poll, so a test expects none." (declare (indent 1)) `(let ((calls '()) (timers '())) @@ -733,8 +729,8 @@ the functions scheduled." (defconst crs-test--ai-feature '((id . "comments-addressed") (name . "Comments addressed?"))) -(ert-deftest crs-test-ai-runs-a-feature-that-never-ran-and-polls () - "Opening a feature with no result asks for a run, then polls while pending." +(ert-deftest crs-test-ai-runs-a-feature-that-never-ran () + "Opening a feature with no result asks for a run, then waits to hear it land." (crs-test--with-fake-rpc `(("RPCHandler.GetAIOutput" . ((output . ((comments-addressed @@ -753,13 +749,14 @@ the functions scheduled." ;; Not forced: the server may answer from its cache. (should (equal (json-encode (cdar calls)) "[{\"Owner\":\"acme\",\"Repo\":\"widgets\",\"Number\":42,\"Feature\":\"comments-addressed\"}]")) - (should (= (length timers) 1)) + ;; Nothing polls: the server says when the run lands. + (should-not timers) (with-current-buffer buffer (should (string-match-p "Running" (buffer-string))))) (kill-buffer buffer))))) (ert-deftest crs-test-ai-current-report-needs-no-run () - "A current report is shown without a run or a poll." + "A current report is shown without a run." (crs-test--with-fake-rpc `(("RPCHandler.GetAIOutput" . ((output . ((comments-addressed . ,(crs-test--ai-output))))))) @@ -843,7 +840,7 @@ the functions scheduled." (let ((buffer (get-buffer (crs--diagram-buffer-name "acme" "widgets" 42)))) (unwind-protect (progn - ;; A current diagram needs no run and no poll. + ;; A current diagram needs no run. (should (equal (mapcar #'car calls) '("RPCHandler.GetAIOutput"))) (should (equal (json-encode (cdar calls)) "[{\"Owner\":\"acme\",\"Repo\":\"widgets\",\"Number\":42,\"Feature\":\"change-diagram\"}]")) @@ -861,7 +858,7 @@ the functions scheduled." (kill-buffer buffer))))) (ert-deftest crs-test-diagram-runs-when-never-drawn () - "A PR with no diagram yet asks for a run and polls; the buffer stays Mermaid." + "A PR with no diagram yet asks for a run; the buffer stays Mermaid." (let ((none '((body_type . "markdown") (body_content . "")))) (crs-test--with-fake-rpc `(("RPCHandler.GetAIOutput" @@ -876,7 +873,7 @@ the functions scheduled." (crs--ai-fetch buffer t) (should (equal (mapcar #'car (reverse calls)) '("RPCHandler.GetAIOutput" "RPCHandler.RunAIFeature"))) - (should (= (length timers) 1)) + (should-not timers) (should (string-match-p "\\`%% Drawing the diagram" (buffer-string))) (should (string-match-p "Status: pending" header-line-format)))))) @@ -951,15 +948,180 @@ the functions scheduled." (should (eq (lookup-key crs-diagram-view-mode-map (kbd "R")) #'crs-ai-rerun)) (should (eq (lookup-key crs-diagram-view-mode-map (kbd "q")) #'crs-quit-ai-output))) -(ert-deftest crs-test-diagram-killing-the-buffer-stops-polling () - "A pending poll timer does not outlive the diagram buffer." - (let ((buffer (cl-letf (((symbol-function 'pop-to-buffer) #'ignore)) - (crs--diagram-open "acme" "widgets" 42)))) - (with-current-buffer buffer - (setq crs--ai-poll-timer (run-with-timer 3600 nil #'ignore))) - (let ((timer (buffer-local-value 'crs--ai-poll-timer buffer))) - (kill-buffer buffer) - (should-not (memq timer timer-list))))) +(ert-deftest crs-test-diagram-redraws-when-the-run-lands () + "The diagram buffer redraws when the server announces the diagram's run." + (let ((replies `(("RPCHandler.GetAIOutput" + . ((output . ((change-diagram + . ,(crs-test--diagram-output 'status "pending" + 'report nil 'updated_at ""))))))))) + (crs-test--with-fake-rpc replies + (crs-test--in-diagram-buffer + (crs--ai-fetch buffer nil) + (should (string-match-p "\\`%% Drawing the diagram" (buffer-string))) + (setq replies `(("RPCHandler.GetAIOutput" + . ((output . ((change-diagram . ,(crs-test--diagram-output)))))))) + (crs--handle-notification + "Server.AIOutputChanged" + (vector '((owner . "acme") (repo . "widgets") (number . 42) + (feature . "change-diagram") (status . "success")))) + (should (equal (buffer-string) (concat crs-test--diagram-source "\n"))) + (should-not timers))))) + +;;; --- Server notifications --- + +(defun crs-test--notify (method data) + "Feed the filter a notification line naming METHOD with DATA, as the server writes it." + (crs--handle-response + (json-encode `((method . ,method) (params . ,(vector data)) (id . nil))))) + +(ert-deftest crs-test-notifications-reach-their-handlers () + "A notification goes to the handlers; a reply still goes to its callback." + (let ((crs-server-notification-functions nil) + (crs--pending-requests (make-hash-table :test 'equal)) + (heard '()) + (replied nil)) + (add-hook 'crs-server-notification-functions + (lambda (method data) (push (cons method data) heard))) + ;; One handler failing doesn't stop the others. + (add-hook 'crs-server-notification-functions + (lambda (&rest _) (error "Boom"))) + (cl-letf (((symbol-function 'message) #'ignore)) + (crs-test--notify "Server.PluginOutputChanged" + '((owner . "acme") (repo . "widgets") (number . 42) + (plugin . "lint") (status . "success")))) + (should (equal heard + '(("Server.PluginOutputChanged" + (owner . "acme") (repo . "widgets") (number . 42) + (plugin . "lint") (status . "success"))))) + (puthash 7 (lambda (result) (setq replied result)) crs--pending-requests) + (crs--handle-response "{\"id\":7,\"result\":{\"okay\":true},\"error\":null}") + (should (equal replied '((okay . t)))) + (should (= (length heard) 1)))) + +(ert-deftest crs-test-same-pr-ignores-case () + "A notification names a PR as GitHub does: owner and repo in any case." + (let ((data '((owner . "Acme") (repo . "Widgets") (number . 42)))) + (should (crs--server-same-pr-p data "acme" "widgets" 42)) + (should-not (crs--server-same-pr-p data "acme" "widgets" 43)) + (should-not (crs--server-same-pr-p data "acme" "gadgets" 42)) + (should-not (crs--server-same-pr-p data nil nil 42)))) + +(ert-deftest crs-test-ai-buffer-follows-announced-runs () + "An AI buffer reads its output again when the server announces its feature's run." + (let ((replies `(("RPCHandler.GetAIOutput" + . ((output . ((comments-addressed + . ,(crs-test--ai-output 'status "pending" 'report nil + 'body '((body_content . "")))))))))) + (change '((owner . "Acme") (repo . "widgets") (number . 42) + (feature . "comments-addressed") (status . "success")))) + (crs-test--with-fake-rpc replies + (let ((buffer (crs--ai-open "acme" "widgets" 42 crs-test--ai-feature))) + (unwind-protect + (progn + (crs--ai-fetch buffer nil) + (should (string-match-p "Running" (with-current-buffer buffer (buffer-string)))) + (setq calls nil + replies `(("RPCHandler.GetAIOutput" + . ((output . ((comments-addressed . ,(crs-test--ai-output)))))))) + ;; Another feature, or another PR, is none of this buffer's business. + (crs-test--notify "Server.AIOutputChanged" + '((owner . "acme") (repo . "widgets") (number . 42) + (feature . "feature-flags") (status . "success"))) + (crs-test--notify "Server.AIOutputChanged" + '((owner . "acme") (repo . "widgets") (number . 43) + (feature . "comments-addressed") (status . "success"))) + (should-not calls) + (crs-test--notify "Server.AIOutputChanged" change) + (should (equal (mapcar #'car calls) '("RPCHandler.GetAIOutput"))) + (with-current-buffer buffer + (should (string-match-p "1 outstanding of 1 item" (buffer-string)))) + (should-not timers)) + (kill-buffer buffer)))))) + +(ert-deftest crs-test-ai-buffers-catch-up-with-a-new-server () + "A new server's subscription has every AI buffer read its output again." + (crs-test--with-fake-rpc + `(("RPCHandler.Subscribe" . ((seq . 0))) + ("RPCHandler.GetAIOutput" + . ((output . ((comments-addressed . ,(crs-test--ai-output))))))) + (let ((buffer (crs--ai-open "acme" "widgets" 42 crs-test--ai-feature))) + (unwind-protect + (progn + (crs--subscribe) + (should (equal (json-encode (cdr (assoc "RPCHandler.Subscribe" calls))) + "[{\"IntervalSeconds\":300}]")) + (should (member "RPCHandler.GetAIOutput" (mapcar #'car calls))) + (with-current-buffer buffer + (should (string-match-p "1 outstanding of 1 item" (buffer-string))))) + (kill-buffer buffer))))) + +(defun crs-test--plugin-reply (status content) + "A GetPluginOutput reply holding the `lint' plugin with STATUS and CONTENT." + `((output . ((lint . ((status . ,status) (result . ,content) + (body . ((body_type . "markdown") (body_content . ,content))) + (annotations . []))))))) + +(defmacro crs-test--in-plugin-buffer (&rest body) + "Run BODY in a plugin output buffer for acme/widgets #42, then kill it. +The buffer is bound to `buffer'." + (declare (indent 0)) + `(let ((buffer (get-buffer-create "* Plugin Output acme/widgets #42 *"))) + (unwind-protect + (progn + (with-current-buffer buffer + (crs-plugin-output-mode) + (setq crs--plugin-owner "acme" + crs--plugin-repo "widgets" + crs--plugin-number 42)) + ,@body) + (kill-buffer buffer)))) + +(ert-deftest crs-test-plugin-buffer-follows-announced-changes () + "A plugin output buffer reads its output again for each change to its PR's plugins." + (crs-test--with-held-rpc + (crs-test--in-plugin-buffer + (crs-test--notify "Server.PluginOutputChanged" + '((owner . "acme") (repo . "gadgets") (number . 42) + (plugin . "lint") (status . "pending"))) + (should-not sent) + (crs-test--notify "Server.PluginOutputChanged" + '((owner . "acme") (repo . "widgets") (number . 42) + (plugin . "lint") (status . "pending"))) + (should (equal (mapcar #'car sent) '("RPCHandler.GetPluginOutput"))) + ;; A burst while that read is in flight asks for one more read, not one each. + (dolist (status '("success" "success")) + (crs-test--notify "Server.PluginOutputChanged" + `((owner . "acme") (repo . "widgets") (number . 42) + (plugin . "lint") (status . ,status)))) + (should (= (length sent) 1)) + (funcall (nth 2 (car sent)) (crs-test--plugin-reply "pending" "")) + (should (= (length sent) 2)) + (funcall (nth 2 (car sent)) (crs-test--plugin-reply "success" "All clear.")) + (should (= (length sent) 2)) + (with-current-buffer buffer + (should (string-match-p "Status: success" (buffer-string))) + (should (string-match-p "All clear\\." (buffer-string)))) + ;; Followed quietly: nothing was said in the echo area. + (should-not (assq 'message events))))) + +(ert-deftest crs-test-plugin-run-asked-for-is-announced () + "A plugin run asked for from here is announced when it lands, and only once." + (clrhash crs--plugin-runs-requested) + (puthash (crs--plugin-run-key "acme" "widgets" 42 "lint") t crs--plugin-runs-requested) + (crs-test--with-held-rpc + (crs-test--notify "Server.PluginOutputChanged" + '((owner . "acme") (repo . "widgets") (number . 42) + (plugin . "lint") (status . "pending"))) + (should-not events) + (crs-test--notify "Server.PluginOutputChanged" + '((owner . "acme") (repo . "widgets") (number . 42) + (plugin . "lint") (status . "success"))) + (should (string-match-p "\\`Plugin 'lint' finished for acme/widgets #42" + (cdr (assq 'message events)))) + (crs-test--notify "Server.PluginOutputChanged" + '((owner . "acme") (repo . "widgets") (number . 42) + (plugin . "lint") (status . "success"))) + (should (= (length events) 1)))) ;;; --- Submitting a review --- diff --git a/client.el/crs-vars.el b/client.el/crs-vars.el index 8f7fd9ec..ba716abf 100644 --- a/client.el/crs-vars.el +++ b/client.el/crs-vars.el @@ -160,6 +160,33 @@ While one is shown, `crs--buffer-diff' holds the filtered diff.") (defvar-local crs--plugin-name nil "If non-nil, this buffer displays output only for this plugin.") +(defvar-local crs--plugin-refreshing nil + "Non-nil while this plugin output buffer is fetching its output again. +It is `again' when another refresh was asked for meanwhile, which runs +once the one in flight lands.") + +(defvar crs--plugin-runs-requested (make-hash-table :test 'equal) + "Plugin runs asked for from this Emacs, keyed \"OWNER/REPO#NUMBER:PLUGIN\". +Each is announced in the echo area when the server says it landed.") + +;; Server notifications (crs-rpc.el). +(defvar crs-server-notification-functions nil + "Functions called with each notification the server sends. +Each is called with two arguments: the notification's METHOD, such as +\"Server.PluginOutputChanged\", and its DATA, the notification's +params[0] as an alist. The server sends them once `crs-start-server' +has subscribed (RPCHandler.Subscribe).") + +(defvar crs-server-subscribed-hook nil + "Hook run each time a newly started server accepts the subscription. +Whatever it did before then was never announced, so buffers showing its +output read it again.") + +(defconst crs--heartbeat-seconds 300 + "The heartbeat interval asked for when subscribing. +This client does nothing with the server's heartbeat, so it asks for the +slowest one the server allows.") + ;; AI features (crs-ai.el). (defvar crs-ai-features nil "The AI features the server has enabled, as ListAIFeatures reports them. @@ -167,17 +194,6 @@ Only those with a report to open: the ones the server applies itself are left out. Each entry is an alist with at least `id' and `name'. Nil until fetched.") -(defcustom crs-ai-poll-interval 2 - "Seconds between polls of an AI feature run that is still pending." - :type 'number - :group 'crs) - -(defcustom crs-ai-poll-timeout 360 - "Stop polling a pending AI feature run after this many seconds. -The server gives a run five minutes, so this leaves room for one to land." - :type 'number - :group 'crs) - ;; Buffer-local state for AI output buffers. (defvar-local crs--ai-owner nil "Owner of the PR an AI output buffer shows.") @@ -189,10 +205,6 @@ The server gives a run five minutes, so this leaves room for one to land." "The feature (an alist from ListAIFeatures) an AI output buffer shows.") (defvar-local crs--ai-output nil "The last output the AI output buffer rendered, from GetAIOutput.") -(defvar-local crs--ai-poll-timer nil - "Timer polling a pending run for this AI output buffer.") -(defvar-local crs--ai-poll-started nil - "When the current round of polling began, as a float time.") (defvar-local crs--ai-insert-function nil "Function inserting an output into this AI buffer, or nil for a report. It takes one argument, the feature's entry of a GetAIOutput reply. Nil diff --git a/docs/ai_features.md b/docs/ai_features.md index 4689060b..144d0c89 100644 --- a/docs/ai_features.md +++ b/docs/ai_features.md @@ -646,7 +646,8 @@ The `report` is `{"rating": "easy" | "medium" | "hard"}`. legend) to share on **Copy image** / **Download image**; the AI page draws it inline. - **Emacs.** `C` in a review buffer (`crs-get-ai-output`) opens the report of - an enabled feature in its own buffer, which polls while a run is pending. In + an enabled feature in its own buffer, which redraws as the server announces + the feature's runs starting and landing, without polling. In that buffer, `r` refreshes, `R` re-runs and `q` quits. The applied features aren't offered; review-ease shows as the headline tag in the reviews buffer. The change diagram has a dedicated command, `crs-show-change-diagram` (`M` in diff --git a/docs/building_clients.md b/docs/building_clients.md index 49230de4..502c4233 100644 --- a/docs/building_clients.md +++ b/docs/building_clients.md @@ -6,7 +6,7 @@ If you want to build a new client (e.g. for VS Code, Vim, or a TUI), you should The server binary `crs` (installed by `go install ./...`; `codereviewserver` before the module was renamed) should be spawned as a child process by your client. Your client send requests to the server's `stdin` and reads responses from `stdout`. -To hear from the server without polling it, call `RPCHandler.Subscribe`: the server then sends a `Server.Heartbeat` notification every interval (and any other event it pushes) on the same `stdout`. Tell a notification from a response by its `method`, and treat a run of missed heartbeats as the server being gone. See [Server Notifications](protocol.md#server-notifications). +To hear from the server without polling it, call `RPCHandler.Subscribe`: the server then sends a `Server.Heartbeat` notification every interval, and `Server.PluginOutputChanged` / `Server.AIOutputChanged` as each plugin or AI run for a PR starts and lands, on the same `stdout`. Tell a notification from a response by its `method`, read `GetPluginOutput` / `GetAIOutput` again when a change names the PR you are showing (and once after subscribing, to catch up), and treat a run of missed heartbeats as the server being gone. See [Server Notifications](protocol.md#server-notifications). If you are building a **web** client, [Web Client Features](web_client_features.md) inventories everything the shipped `bun_client` does — the HTTP/WebSocket bridge that fronts the stdio server, the full feature list, which protocol methods it uses (and which it doesn't), and the gotchas worth knowing before you reimplement diff parsing or comment positions. diff --git a/docs/clients.md b/docs/clients.md index 7c66b488..492d50df 100644 --- a/docs/clients.md +++ b/docs/clients.md @@ -157,7 +157,7 @@ feedback in place to submit again. When the server has an [AI feature](ai_features.md) enabled, `C` in a review buffer (`crs-get-ai-output`) opens its report for the PR — for example, which review comments are still outstanding — running it first when it never ran or its -report is stale. The report buffer updates itself while a run is pending; `r` +report is stale. The report buffer updates itself as runs start and land; `r` refreshes it, `R` forces a re-run and `q` quits. `F` in a review buffer (`crs-show-filtered-diff`) narrows the diff to a @@ -172,6 +172,16 @@ nothing to start from — you never reviewed the PR, or a force-push rewrote the commit you last saw — the full diff stays and the header says why. Syncing or submitting a review keeps the filter and asks for it again. +The client follows the server's runs rather than polling for them: +`crs-start-server` subscribes to the server's +[notifications](protocol.md#server-notifications), and each time the server +announces a change to a PR's plugin output or AI results, every open buffer +showing it — a plugin output buffer, an AI report, the change diagram — reads it +again, keeping point where it was. A plugin run with `crs-rerun-plugin` or +`crs-run-on-demand-plugin` is announced in the echo area when it lands. The hooks +`crs-server-notification-functions` (called with each notification's method and +data) and `crs-server-subscribed-hook` are there for your own code too. + The [change diagram](ai_features.md#change-diagram) has a command of its own: `M` in a review buffer, or on a PR's line in the reviews buffer, runs `crs-show-change-diagram`, which shows the diagram's raw Mermaid source in a From 00dcaddd7270303d28bf76c0b2d67163963753ab Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 26 Oct 2026 13:35:52 +0000 Subject: [PATCH 4/5] Keep a lost reply from stalling an Emacs plugin buffer; note the change events in CLAUDE.md An announced change is fetched one at a time per plugin buffer, but a reply that never comes (the server died mid-request) would have left the buffer marked as fetching for good, swallowing even a refresh by hand. A refresh asked for by hand now always fetches, and a new server's subscription clears the mark before catching the buffers up. CLAUDE.md describes the two change events, the AI runner's OnChange hook, and how each client follows them instead of polling. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011prZsf8nHYe7e2Ke1rrq6z --- CLAUDE.md | 10 +++++----- client.el/crs-plugins.el | 12 ++++++++---- client.el/crs-tests.el | 11 ++++++++++- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e1233139..43f1f09f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -50,7 +50,7 @@ Periodic background jobs fetch PRs from GitHub, apply filters, and persist resul ### Server Layer (`server/`) RPC handlers serve data to clients (web UI, Emacs). - `server.go` — RPC method dispatch (`GetAllReviews`, `GetPR`, `AddComment`, `SubmitReview`, the AI RPCs `ListAIFeatures`/`RunAIFeature`/`GetAIOutput`, etc.) -- `notify.go` — server → client notifications (docs/protocol.md#server-notifications): a client that calls `Subscribe` gets JSON-RPC notifications (`id: null`, named by `method`) on the stdout its replies use — a `Server.Heartbeat` every interval, and whatever goes through `Notifier.Notify`. Replies and notifications share one `syncWriter`, so lines never splice; a client that never subscribes never sees one (the TUI reads one line per call) +- `notify.go` — server → client notifications (docs/protocol.md#server-notifications): a client that calls `Subscribe` gets JSON-RPC notifications (`id: null`, named by `method`) on the stdout its replies use — a `Server.Heartbeat` every interval, and the events that go through `Notifier.Notify`: `Server.PluginOutputChanged` each time a plugin result is written (`storePluginResult`, plugins.go) or cleared by `RerunPlugins`, and `Server.AIOutputChanged` as an AI run is claimed and as its result is stored (the `ai.Runner`'s `OnChange`, set in `RunServer`). Each names the PR and plugin/feature with its new status, never the output; clients re-read `GetPluginOutput` / `GetAIOutput` on them instead of polling. Replies and notifications share one `syncWriter`, so lines never splice; a client that never subscribes never sees one (the TUI reads one line per call) - `renderer.go` — `GetPRDetails()` fetches PR metadata/diff/comments/reviews, checking DB caches first; `aiDiscussion` partitions its comments into threads/conversation for the AI features - `plugins.go` — async plugin execution - `hooks.go` — the post-update hooks (plugins, automatic AI features), the request for the applied AI features when a client opens a PR (`requestAppliedAIFeatures`), and AI request assembly @@ -65,7 +65,7 @@ RPC handlers serve data to clients (web UI, Emacs). - `images/` — downloads and caches the images embedded in PR bodies, reviews and comments to `$CRS_HOME/images`, content-addressed by URL. Only `github.com` / `*.githubusercontent.com` are fetched, since bodies are attacker-supplied text. Clients render from the cached file rather than from GitHub: a private repo's attachments are served only to a request carrying the server's token - `llm/` — the text-generation `Client` interface built by `NewClient`, the stages a call fails at (`CallError`), and the call log: the AI runner appends every run to `~/.crs/llm_calls.log` via `AppendCallReport`. Two HTTP backends: Gemini (model pinned) and OpenRouter (`NewOpenRouterClient(model)`, a `ContextClient` so a deadline cancels the request) - `openrouter/` — the OpenRouter chat-completions client, shared by `llm` and the bundled plugins (`cmd/internal/pluginkit`). It imports nothing from the module, so plugin binaries don't link the server's config/database (cgo sqlite) -- `ai/` — the AI feature layer (docs/ai_features.md): a `Registry` of `Feature`s (`comments-addressed`; `feature-flags`, which judges whether each hunk's logic change is behind a feature flag, a hunk that only adds definitions being judged where it's called; `change-diagram`, whose report is the raw Mermaid source of a flowchart of the PR's changes, which clients render themselves; and the applied features `file-ordering` and `review-ease`, an `AppliedFeature` being one whose result the server applies to what it serves — the diff's file order, the list's ease rating — rather than a report a client opens), the `Provider` (one-shot: Gemini or OpenRouter via `llm.Client`, or a command-backed CLI agent; `config.AIProviderFor` resolves the provider plus the command/model it reads) and `Agent` (tool loop on any provider) seams, `Request.FilteredDiff` (the PR's diff since the last review/interaction, from the filtered-diff cache), and the `Runner` — cache check keyed by head SHA + `InputsDigest`, in-memory in-flight tracking (so "pending" never outlives a restart), a cap of two concurrent automatic runs, a per-feature timeout, and one call-log entry per run. A `CodeOnlyFeature` is keyed by head SHA alone (`KeyDigest`), so comments don't make it stale. Results go to the `AIResults` table. Configured by `[AI]`/`[[AIFeatures]]`, all off by default; the legacy root-level `ExperimentalLLM*` keys still work — `config.AIFeatureSettings` reads a legacy flag as an automatic entry for file-ordering or review-ease (`legacyLLMKeys`), so ask it, not `AIFeatures`, whether a feature is on +- `ai/` — the AI feature layer (docs/ai_features.md): a `Registry` of `Feature`s (`comments-addressed`; `feature-flags`, which judges whether each hunk's logic change is behind a feature flag, a hunk that only adds definitions being judged where it's called; `change-diagram`, whose report is the raw Mermaid source of a flowchart of the PR's changes, which clients render themselves; and the applied features `file-ordering` and `review-ease`, an `AppliedFeature` being one whose result the server applies to what it serves — the diff's file order, the list's ease rating — rather than a report a client opens), the `Provider` (one-shot: Gemini or OpenRouter via `llm.Client`, or a command-backed CLI agent; `config.AIProviderFor` resolves the provider plus the command/model it reads) and `Agent` (tool loop on any provider) seams, `Request.FilteredDiff` (the PR's diff since the last review/interaction, from the filtered-diff cache), and the `Runner` — cache check keyed by head SHA + `InputsDigest`, in-memory in-flight tracking (so "pending" never outlives a restart), a cap of two concurrent automatic runs, a per-feature timeout, one call-log entry per run, and an `OnChange` hook told as a run is claimed (pending) and as its result is stored. A `CodeOnlyFeature` is keyed by head SHA alone (`KeyDigest`), so comments don't make it stale. Results go to the `AIResults` table. Configured by `[AI]`/`[[AIFeatures]]`, all off by default; the legacy root-level `ExperimentalLLM*` keys still work — `config.AIFeatureSettings` reads a legacy flag as an automatic entry for file-ordering or review-ease (`legacyLLMKeys`), so ask it, not `AIFeatures`, whether a feature is on - `logger/` — the slog handler: text (`DATE::TIME - LEVEL - msg key=value`) or JSON (`CRS_LOG_FORMAT`), to stderr and optionally a file (`CRS_LOG_FILE`, moved aside at startup past 64 MB), at `CRS_LOG_LEVEL` - `subprocess/` — runs a command with a timeout, capturing stdout/stderr; shared by plugins and the command provider - `org/` — org-mode serialization for the Emacs client @@ -73,9 +73,9 @@ RPC handlers serve data to clients (web UI, Emacs). - `cmd/` — plugin binaries (summarize_diff, security_check, etc.). The bundled LLM plugins call `pluginkit.ModelFromEnv()`, which picks Gemini or OpenRouter from `CRS_LLM_PROVIDER`/`CRS_LLM_MODEL` — set by the server from a `[[Plugins]]` entry's `Provider`/`Model` (`config.Plugin.Env`). `style_guidelines` is agentic instead: it drives its own two-phase tool loop (review, then validate the findings) over `pluginkit.Chat`, which speaks Gemini function calling and OpenRouter tool calls natively, and reads its guide from a file or a directory (`CRS_STYLE_GUIDE_DIR`) ### Clients -- `bun_client/` — Bun + React web UI. `server.ts` bridges HTTP/WebSocket to the Go backend's stdio — it subscribes to the server's notifications at startup and `server_events.ts` relays them to the page as Server-Sent Events on `GET /api/events`, where one shared `EventSource` (`frontend/src/server_events.ts`) feeds the top bar's heartbeat status — and `lsp_pool.ts` keeps language servers (`diff-lsp`, `gopls`, ...) running across LSP WebSockets so reopening a review doesn't re-index the workspace. The review toolbar's diff-range select swaps the diff for a filtered one (`FilteredDiffBanner.tsx`, `diff_filter_utils.ts`); `Review.tsx` keeps the full diff in `fullDiff` for comment positions, which a filtered diff doesn't share. The change diagram is drawn with the `mermaid` library (`MermaidDiagram.tsx`), imported lazily in `mermaid_utils.ts` so it stays out of the main bundle; its Copy image / Download image buttons rasterize the drawn SVG to a PNG on a canvas (`mermaid_image.ts`), made in the background once the diagram is drawn and kept for both, within a pixel budget and time limits -- `client.el/` — Emacs client, split into modules with `crs-client.el` as the entry point (`crs-vars`, `crs-html`, `crs-rpc`, `crs-filtered-diff`, `crs-render`, `crs-list-mode`, `crs-review`, `crs-comments`, `crs-review-actions`, `crs-plugins`, `crs-ai`, `crs-diagram`). `crs-filtered-diff` (`F`) swaps a review buffer's `crs--buffer-diff` for a filtered one, keeping the full diff in `crs--buffer-full-diff`; its buffer-locals are `permanent-local` so re-entering the major mode on render keeps them, and `crs--render-and-update` skips inline comments while one is displayed and re-requests it on a fresh payload. `crs-diagram` shows the change diagram in a `mermaid-mode` buffer (an optional dependency) through `crs-ai`'s fetch/poll machinery (`crs--ai-insert-function`). A new module must also be added to the byte-compile list in `.github/workflows/all-checks.yml` -- `chrome_extension/` — MV3 Chrome extension that shows the AI features and plugins for the GitHub PR a tab is on (any PR, listed or not), and the review list elsewhere; not a review UI (README.md). The service worker (`background.ts`, `native_client.ts`) owns the one native port, opened on the first RPC; one port is one `crs_native_host` and one `crs --server`, kept alive with the browser. `crs_native_host/` (Go, stdlib only, imports nothing from the module) bridges Chrome's length-prefixed frames to the server's newline-delimited JSON-RPC, chunks responses over Chrome's 1 MB host→extension limit into `crs_chunk` streams (`native_protocol.ts` reassembles them), reports `crs_host` ready/error/server_exited events, and merges `~/.crs/native_host.env` first because Chrome launches hosts without the shell's environment; `install.sh` registers it for the extension ID the manifest `key` pins. `content.ts` (IIFE, small, no RPC) toggles a modal over github.com holding the panel (`src/panel/`, React) in an iframe, passing `?owner=&repo=&number=` on a PR page; off GitHub the panel opens as a `?standalone=1` popup window. The service worker answers only extension pages and forwards only `RPC_METHODS` (`types.ts`): read-and-run RPCs, no review/merge/config mutations. The content script also draws the chips beside a PR's tabs, and on a Files changed view lays the stored file-ordering result over GitHub's own diffs and file tree (`file_order.ts`); both come from read-only messages the service worker answers for the sender tab's own PR (`crs-tools`, `crs-file-order`). +- `bun_client/` — Bun + React web UI. `server.ts` bridges HTTP/WebSocket to the Go backend's stdio — it subscribes to the server's notifications at startup and `server_events.ts` relays them to the page as Server-Sent Events on `GET /api/events`, where one shared `EventSource` (`frontend/src/server_events.ts`) feeds the top bar's heartbeat status and `useServerChanges`, which the plugin and AI views (`usePluginOutputs`, `useAIReport`, `useAIBackgroundRuns`) use to re-read their output when a change names their PR, and on reconnect, instead of polling — and `lsp_pool.ts` keeps language servers (`diff-lsp`, `gopls`, ...) running across LSP WebSockets so reopening a review doesn't re-index the workspace. The review toolbar's diff-range select swaps the diff for a filtered one (`FilteredDiffBanner.tsx`, `diff_filter_utils.ts`); `Review.tsx` keeps the full diff in `fullDiff` for comment positions, which a filtered diff doesn't share. The change diagram is drawn with the `mermaid` library (`MermaidDiagram.tsx`), imported lazily in `mermaid_utils.ts` so it stays out of the main bundle; its Copy image / Download image buttons rasterize the drawn SVG to a PNG on a canvas (`mermaid_image.ts`), made in the background once the diagram is drawn and kept for both, within a pixel budget and time limits +- `client.el/` — Emacs client, split into modules with `crs-client.el` as the entry point (`crs-vars`, `crs-html`, `crs-rpc`, `crs-filtered-diff`, `crs-render`, `crs-list-mode`, `crs-review`, `crs-comments`, `crs-review-actions`, `crs-plugins`, `crs-ai`, `crs-diagram`). `crs-filtered-diff` (`F`) swaps a review buffer's `crs--buffer-diff` for a filtered one, keeping the full diff in `crs--buffer-full-diff`; its buffer-locals are `permanent-local` so re-entering the major mode on render keeps them, and `crs--render-and-update` skips inline comments while one is displayed and re-requests it on a fresh payload. `crs-diagram` shows the change diagram in a `mermaid-mode` buffer (an optional dependency) through `crs-ai`'s fetch machinery (`crs--ai-insert-function`). `crs-start-server` subscribes to notifications, `crs-rpc` hands them to `crs-server-notification-functions`, and open plugin, AI and diagram buffers re-read their PR's output on them (nothing polls). A new module must also be added to the byte-compile list in `.github/workflows/all-checks.yml` +- `chrome_extension/` — MV3 Chrome extension that shows the AI features and plugins for the GitHub PR a tab is on (any PR, listed or not), and the review list elsewhere; not a review UI (README.md). The service worker (`background.ts`, `native_client.ts`) owns the one native port, opened on the first RPC; one port is one `crs_native_host` and one `crs --server`, kept alive with the browser. It subscribes each new server first and relays `Server.PluginOutputChanged` / `Server.AIOutputChanged` (`server_events.ts`) to extension pages on a `crs-events` port (`usePRChanges`; PRTools re-reads on them, never polls), plus a `resync` when a new server subscribes, and tells a PR's tabs when a file order lands (`crs-file-order-changed`). `crs_native_host/` (Go, stdlib only, imports nothing from the module) bridges Chrome's length-prefixed frames to the server's newline-delimited JSON-RPC, chunks responses over Chrome's 1 MB host→extension limit into `crs_chunk` streams (`native_protocol.ts` reassembles them), reports `crs_host` ready/error/server_exited events, and merges `~/.crs/native_host.env` first because Chrome launches hosts without the shell's environment; `install.sh` registers it for the extension ID the manifest `key` pins. `content.ts` (IIFE, small, no RPC) toggles a modal over github.com holding the panel (`src/panel/`, React) in an iframe, passing `?owner=&repo=&number=` on a PR page; off GitHub the panel opens as a `?standalone=1` popup window. The service worker answers only extension pages and forwards only `RPC_METHODS` (`types.ts`): read-and-run RPCs, no review/merge/config mutations. The content script also draws the chips beside a PR's tabs, and on a Files changed view lays the stored file-ordering result over GitHub's own diffs and file tree (`file_order.ts`); both come from read-only messages the service worker answers for the sender tab's own PR (`crs-tools`, `crs-file-order`). ## Key Data Flow diff --git a/client.el/crs-plugins.el b/client.el/crs-plugins.el index 7d8b9b36..37b6aaba 100644 --- a/client.el/crs-plugins.el +++ b/client.el/crs-plugins.el @@ -64,11 +64,12 @@ listed beneath the body sorted by file and line." (defun crs--refresh-plugin-buffer (buffer &optional quiet) "Fetch the plugin output shown in BUFFER again and redraw it. With QUIET, as for a change the server announced, say nothing and keep -point where the reader left it. One fetch runs at a time per buffer: -one asked for meanwhile runs once it lands." +point where the reader left it. Announced changes are fetched one at a +time per buffer: one that comes in meanwhile is fetched once that lands. +A refresh asked for by hand always fetches." (when (buffer-live-p buffer) (with-current-buffer buffer - (if crs--plugin-refreshing + (if (and quiet crs--plugin-refreshing) (setq crs--plugin-refreshing 'again) (setq crs--plugin-refreshing t) (let ((owner crs--plugin-owner) @@ -161,9 +162,12 @@ run asked for from here is announced once it lands." (defun crs--plugin-on-subscribed () "Read every open plugin output buffer's output again, for a new server. -Runs asked for from the old one are gone with it." +Runs asked for from the old one are gone with it, and so are any replies +it owed." (clrhash crs--plugin-runs-requested) (dolist (buffer (crs--plugin-buffers)) + (with-current-buffer buffer + (setq crs--plugin-refreshing nil)) (crs--refresh-plugin-buffer buffer t))) (add-hook 'crs-server-notification-functions #'crs--plugin-on-notification) diff --git a/client.el/crs-tests.el b/client.el/crs-tests.el index 7bb78529..ff9e6de5 100644 --- a/client.el/crs-tests.el +++ b/client.el/crs-tests.el @@ -1102,7 +1102,16 @@ The buffer is bound to `buffer'." (should (string-match-p "Status: success" (buffer-string))) (should (string-match-p "All clear\\." (buffer-string)))) ;; Followed quietly: nothing was said in the echo area. - (should-not (assq 'message events))))) + (should-not (assq 'message events)) + ;; A refresh asked for by hand fetches even with a read in flight, so a + ;; reply that never came can't leave the buffer stuck. + (crs-test--notify "Server.PluginOutputChanged" + '((owner . "acme") (repo . "widgets") (number . 42) + (plugin . "lint") (status . "pending"))) + (should (= (length sent) 3)) + (with-current-buffer buffer + (crs-refresh-plugin-output)) + (should (= (length sent) 4))))) (ert-deftest crs-test-plugin-run-asked-for-is-announced () "A plugin run asked for from here is announced when it lands, and only once." From c93266c02c09b6a40b7ac7378649fa10f5b04347 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 26 Oct 2026 17:51:49 +0000 Subject: [PATCH 5/5] Match the plugin-landed message whatever the quoting style CI's Emacs curves the quotes `message' writes (text-quoting-style), so the test looking for a straight 'lint' failed there while passing locally. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011prZsf8nHYe7e2Ke1rrq6z --- client.el/crs-tests.el | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/client.el/crs-tests.el b/client.el/crs-tests.el index ff9e6de5..06b23100 100644 --- a/client.el/crs-tests.el +++ b/client.el/crs-tests.el @@ -1125,7 +1125,8 @@ The buffer is bound to `buffer'." (crs-test--notify "Server.PluginOutputChanged" '((owner . "acme") (repo . "widgets") (number . 42) (plugin . "lint") (status . "success"))) - (should (string-match-p "\\`Plugin 'lint' finished for acme/widgets #42" + ;; `message' curves the quotes where `text-quoting-style' says to. + (should (string-match-p "\\`Plugin .lint. finished for acme/widgets #42" (cdr (assq 'message events)))) (crs-test--notify "Server.PluginOutputChanged" '((owner . "acme") (repo . "widgets") (number . 42)