From 9f3e329f8507e4404a15bf10ebc2893817f6f012 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 26 Oct 2026 13:14:40 +0000 Subject: [PATCH 1/2] Refresh the web client live when a webhook delivery changes a PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With [Webhooks] configured the server already patches its caches from each delivery, but an open page only found out on a manual sync or a reload. Now the server pushes a Server.PRUpdated notification (over the channel #303 added) for every PR a delivery changed, and the web client re-reads the PR in place and says what happened. - workflows: each apply function reports what it rewrote for each PR (cache fields, "head" after a refetch, "pr" for a pull_request payload) and a few-word summary ("alice approved", "bob commented on x.go", "alice pushed new commits"). CI is announced only as it settles (passing, or failing where it wasn't); a check starting refreshes quietly. applyWebhookEvent hands each changed PR to a hook registered with SetPRChangedHook, and the membership re-check sends a "sections" change when it rewrote rows. Deliveries that changed nothing send nothing. - server: NotifyPRChanged sends Server.PRUpdated (owner, repo, number, event, changes, head_sha, actor, own, summary, time) to a subscribed client; main.go wires it in server mode. "own" marks the configured user's changes. - web client: onPRUpdates/usePRUpdates batch a burst (400 ms from the first update). The review re-calls GetPR without a loading state and takes in comments, reviews, metadata and a changed diff, keeping open threads, expanded hunks, drafts and the feedback being typed; a new diff waits while an inline comment is being written. The list re-reads its rows in place and highlights the changed ones. A toast says what happened; the user's own changes and quiet ones refresh silently. - e2e: E2E.UpdatePR / backend.updatePR play a delivery; live_updates.e2e.ts covers the review, the held diff, the user's own change, sections-only updates and the list. - docs: protocol.md (Server.PRUpdated), webhooks.md, web_client_features.md §3.18, building_clients.md, CLAUDE.md. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AMPYZg3x4vXfFzqgvJ1AbU --- CLAUDE.md | 7 +- bun_client/e2e/README.md | 5 +- bun_client/e2e/harness/fake_crs.ts | 33 ++ bun_client/e2e/harness/test.ts | 25 ++ bun_client/e2e/live_updates.e2e.ts | 129 ++++++++ bun_client/frontend/src/App.css | 22 ++ bun_client/frontend/src/App.tsx | 1 + bun_client/frontend/src/components/PRList.tsx | 71 ++++- bun_client/frontend/src/components/Review.tsx | 103 +++++- bun_client/frontend/src/hooks/usePRUpdates.ts | 29 ++ .../frontend/src/server_events.test.tsx | 182 +++++++++++ bun_client/frontend/src/server_events.ts | 147 +++++++++ docs/building_clients.md | 2 +- docs/protocol.md | 27 +- docs/web_client_features.md | 33 +- docs/webhooks.md | 4 + main.go | 3 + server/notify.go | 77 ++++- server/notify_test.go | 60 ++++ workflows/webhook_changes.go | 298 ++++++++++++++++++ workflows/webhook_changes_test.go | 293 +++++++++++++++++ workflows/webhook_events.go | 126 +++++--- workflows/webhook_membership.go | 18 +- workflows/webhooks.go | 13 +- 24 files changed, 1645 insertions(+), 63 deletions(-) create mode 100644 bun_client/e2e/live_updates.e2e.ts create mode 100644 bun_client/frontend/src/hooks/usePRUpdates.ts create mode 100644 workflows/webhook_changes.go create mode 100644 workflows/webhook_changes_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 63aeaf7f..6e94f464 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,12 +45,13 @@ Periodic background jobs fetch PRs from GitHub, apply filters, and persist resul - `auxdata.go` — in-memory auxiliary data (reviews, commits, CI status, review threads) used during workflow display - `webhooks.go` / `webhook_events.go` / `webhook_cache.go` — GitHub webhooks (`[Webhooks]`, docs/webhooks.md), off without that table. The process running the sync listens (`syncWebhookListener`, called from `ManagerService.Run` each cycle); `serveWebhook` verifies the signature and queues; one worker applies deliveries in order, patching the DB caches in place for any PR we hold (in a section, or cached by a cycle — the filters read candidates' caches) and never starting a list cache from one delivery. Only a moved head or base calls GitHub (`refetchPR`: a full warm plus the post-update hooks), and a new review thread (its GraphQL ID). `beginWebhookCycle` decides per cycle: polling until the listener is up, for the first cycle after, and every `FullRefreshInterval`; otherwise `webhookHandles(key)` is true for PRs in covered repos, and `planWebhookRefresh` / `prefetchAuxDataForPRs` read those whose caches are current (`cacheServes`) via `loadCachedAuxData` instead of fetching — the rest are warmed in full. `recordMergeability` re-asks only when the head moved or a push moved the base (`mergeabilityCurrent`), and the registered `git_tools.PRStateSource` (`webhookPRState`) answers the interaction and CI filters from the caches. `lockPR` serializes a fetch's read-then-write with a delivery's patch for the same PR. `activeCoverage` (behind `webhookHandles`) holds between cycles while webhooks are trusted: set by a webhook cycle or a completed full refresh, cleared when a cycle refreshes everything or the listener stops - `webhook_membership.go` — after a delivery, the PR is checked against each workflow again (`webhookMember.webhookMembership`: repo/state + filters for Sync, review-request-to-me-or-my-teams for `ModeReviewRequested`, the mode filter for `ModeWaitingOnMe`, only drops for ProjectList) from the caches, and its section rows added/rewritten/dropped via `applyChange`. Queued by key and deduped (`scheduleReevaluation`), run by `runReevaluations` under `membershipMu`, which `ManagerService.Run` holds from a cycle's start until its sections are written and pruned; trust is checked when a check runs, so a delivery that lands mid-cycle is checked once the cycle ends (what makes `FullRefreshInterval <= SleepDuration` — poll every cycle, webhooks only in between — work). Each PR is judged by the newest PR object seen (`rememberPR`, from listings and payloads, newer `updated_at` wins), so a check that waited out a cycle can't undo its listing. A submitted review clears its author's request in the payload's PR (`withoutReviewer`) +- `webhook_changes.go` — telling clients what a delivery changed: each `heldPR` an apply function returns carries the fields it rewrote (`changes`: a cache field, `ChangeHead` after a refetch, `ChangePR` for a pull_request payload) and a few-word `summary` (`reviewSummary`, `ciSummaryChange` — CI is announced only as it settles), and `applyWebhookEvent` hands each PR with changes to the hook registered via `SetPRChangedHook` (`server.NotifyPRChanged`, wired in `main.go`) as a `PRChange`; `reevaluateMembership` sends one with `ChangeSections` when it rewrote rows. Synchronous on the webhook worker, so the hook must not block - `webhook_stats.go` — per-cycle accounting: `takeWebhookActivity` (deliveries applied/ignored/rejected, refetches, their API calls, section adds/drops) and `takeTrafficSinceLastRow` (deltas of `git_tools.GetRequestCounts`, every request counted on the wire by budget), written with the cycle's `mode` (`database.CycleMode*`) into its `APICallStats` row ### 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, a `Server.PRUpdated` (`NotifyPRChanged`) for each PR a webhook delivery changed, and whatever else 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) - `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 @@ -71,7 +72,7 @@ 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 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 +- `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 the live refresh: `usePRUpdates` batches `Server.PRUpdated` bursts, the review re-reads the PR in place (`Review.tsx` `refreshLive`, holding a new diff while a comment is being written) and the list its rows, with a toast saying what happened — 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 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-render`, `crs-list-mode`, `crs-review`, `crs-comments`, `crs-review-actions`, `crs-plugins`, `crs-ai`, `crs-diagram`). `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`). @@ -81,7 +82,7 @@ RPC handlers serve data to clients (web UI, Emacs). 2. **Aux data fetch**: `fetchAuxDataForPR` (manager.go) fetches reviews/commits/CI/review-threads and persists to DB caches, recording each write in `WorkflowActionLog` (workflow name, head SHA, fields written). What gets fetched is the union of what the workflows' filters asked for and what `applyCacheWarmRequirements` adds for a PR that is new or has a new head SHA — the warm covers every field `GetPRDetails` reads, so opening a review is a pure cache hit 3. **Post-update hooks**: for each PR the cycle added or re-fetched after a push, `notifyPRsUpdated` (manager.go) calls the hook registered via `workflows.SetPRUpdatedHook` — `server.WarmPRAnalysis` in server mode — which runs the plugins and any AI features configured `Automatic` in the background so they're cached before anyone opens the review. A client opening a PR (`ensurePostUpdateHooks`) runs the same hooks and also requests the enabled applied AI features, which no client asks for itself 4. **Image prefetch**: `ensurePostUpdateHooks` also hands every image URL found in the description, reviews and comments to `images.Store.Prefetch`, ahead of the head-SHA debounce — a new comment carrying a screenshot doesn't change the SHA. `PRPayload.images` then reports where each one landed, and `GetImage` fetches on demand anything the prefetch hasn't reached -5. **Webhooks** (only with `[Webhooks]`): deliveries patch the same DB caches steps 1–2 write, then re-check the PR's section membership from them, so a webhook cycle lists PRs, warms new or pushed ones, and reads the rest from the caches; without the table every cycle polls as above +5. **Webhooks** (only with `[Webhooks]`): deliveries patch the same DB caches steps 1–2 write, then re-check the PR's section membership from them, so a webhook cycle lists PRs, warms new or pushed ones, and reads the rest from the caches; without the table every cycle polls as above. Each PR a delivery changed is announced to a subscribed client as a `Server.PRUpdated`, and the web client re-reads it (step 6) from those caches 6. **Client request**: RPC `GetPR` → `GetPRDetails` (renderer.go) → checks DB caches → falls back to GitHub API 7. **Rendering**: `OrgRenderer` reads sections/items from DB, sorts by priority, returns org-mode or JSON diff --git a/bun_client/e2e/README.md b/bun_client/e2e/README.md index 4bfee212..53314580 100644 --- a/bun_client/e2e/README.md +++ b/bun_client/e2e/README.md @@ -12,7 +12,10 @@ no GitHub token, network, or SQLite database: The fake `crs` also answers `Subscribe`: it sends a `Server.Heartbeat` every second (the launcher sets `CRS_HEARTBEAT_SECONDS=1`) until a spec calls -`backend.setHeartbeat(false)`, which `E2E.Reset` undoes. +`backend.setHeartbeat(false)`, which `E2E.Reset` undoes. `backend.updatePR(pr, ...)` +plays a webhook delivery: it changes the PR's fixture (a title, a conversation +comment, text swapped in the diff) and sends the `Server.PRUpdated` the server would +(`live_updates.e2e.ts`). 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/harness/fake_crs.ts b/bun_client/e2e/harness/fake_crs.ts index 8fe54229..20c1c26a 100644 --- a/bun_client/e2e/harness/fake_crs.ts +++ b/bun_client/e2e/harness/fake_crs.ts @@ -734,6 +734,39 @@ const handlers: Record unknown> = { if (typeof args.mermaid === 'string') state.diagramSource = args.mermaid; return { okay: true }; }, + + // A webhook delivery changing a PR: apply what it says to the fixture — + // a new title, a conversation comment, a diff with `ReplaceInDiff`'s text + // swapped — then send the Server.PRUpdated notification the server would. + 'E2E.UpdatePR': args => { + const pr = findPR(args); + if (typeof args.Title === 'string') pr.item.title = args.Title; + if (args.Comment) { + pr.comments.push( + comment({ + id: String(state.nextCommentId++), + author: args.Comment.author, + body: args.Comment.body, + created_at: new Date().toISOString(), + }) + ); + } + if (Array.isArray(args.ReplaceInDiff)) { + pr.diff = pr.diff.replace(args.ReplaceInDiff[0], args.ReplaceInDiff[1]); + } + notify('Server.PRUpdated', { + owner: pr.item.owner, + repo: pr.item.repo, + number: pr.item.number, + event: args.Event ?? 'issue_comment.created', + changes: args.Changes ?? ['comments'], + actor: args.Actor ?? '', + own: !!args.Own, + summary: args.Summary ?? '', + time: new Date().toISOString(), + }); + return { okay: true }; + }, }; function respond(id: Request['id'], result: unknown, error: string | null) { diff --git a/bun_client/e2e/harness/test.ts b/bun_client/e2e/harness/test.ts index 2f2b3adb..ca3456ec 100644 --- a/bun_client/e2e/harness/test.ts +++ b/bun_client/e2e/harness/test.ts @@ -84,6 +84,31 @@ class Backend { setDiagram(enabled: boolean, mermaid?: string) { return this.rpc('E2E.SetDiagram', { enabled, mermaid }); } + + /** + * Change a PR as a webhook delivery would, and push the Server.PRUpdated + * notification the server sends for it. + */ + updatePR(pr: { owner: string; repo: string; number: number }, update: PRUpdateFields) { + return this.rpc('E2E.UpdatePR', { + Owner: pr.owner, + Repo: pr.repo, + Number: pr.number, + ...update, + }); + } +} + +export interface PRUpdateFields { + Title?: string; + Comment?: { author: string; body: string }; + /** Swap the first text for the second in the PR's diff. */ + ReplaceInDiff?: [string, string]; + Event?: string; + Changes?: string[]; + Actor?: string; + Own?: boolean; + Summary?: string; } export interface LspMessage { diff --git a/bun_client/e2e/live_updates.e2e.ts b/bun_client/e2e/live_updates.e2e.ts new file mode 100644 index 00000000..99f1b0f0 --- /dev/null +++ b/bun_client/e2e/live_updates.e2e.ts @@ -0,0 +1,129 @@ +import type { Page } from '@playwright/test'; +import { diffRow, expect, openReview, PR42, test } from './harness/test'; + +// When a GitHub webhook delivery changes a PR, the server sends a +// Server.PRUpdated notification, which the bridge relays to the page over +// /api/events. backend.updatePR changes the fake's fixture and sends one. + +/** The page hears updates over its event stream; a heartbeat means it's open. */ +async function connected(page: Page) { + await expect(page.getByRole('img', { name: 'Server connected' })).toBeVisible(); +} + +/** The toast, which is the page's only status role. */ +const toast = (page: Page) => page.getByRole('status'); + +test.describe('live updates', () => { + test('the review refreshes in place and says what changed', async ({ page, backend }) => { + await openReview(page); + await connected(page); + + // Context expanded by hand survives a refresh that leaves the diff be. + const fileOnly = page.getByText('// Greeting helpers used by the CLI entry point.'); + await page + .locator('.diff-hunk-row') + .filter({ hasText: '@@ -3,7 +3,8 @@' }) + .getByTitle('Expand 20 lines before this hunk') + .click(); + await expect(fileOnly).toBeVisible(); + + await backend.updatePR(PR42, { + Comment: { author: 'carol', body: 'Ship it once CI is green.' }, + Actor: 'carol', + Summary: 'carol commented', + }); + + await expect(toast(page)).toHaveText('carol commented'); + await page.getByRole('button', { name: /Discussion/ }).click(); + await expect(page.getByText('Ship it once CI is green.')).toBeVisible(); + await expect(fileOnly).toBeVisible(); + + // Fetched again from the server's caches, not synced with GitHub. + expect(await backend.calls('GetPR')).toHaveLength(2); + expect(await backend.calls('SyncPR')).toHaveLength(0); + }); + + test("the user's own change refreshes without a toast", async ({ page, backend }) => { + await openReview(page); + await connected(page); + + await backend.updatePR(PR42, { + Title: 'Add a greeting helper', + Event: 'pull_request.edited', + Changes: ['pr', 'metadata'], + Actor: 'e2e-reviewer', + Own: true, + Summary: 'e2e-reviewer edited the title', + }); + + await expect(page.getByRole('heading', { name: 'Add a greeting helper' })).toBeVisible(); + await expect(toast(page)).toHaveCount(0); + }); + + test('a new diff waits for the comment being written', async ({ page, backend }) => { + await openReview(page); + await connected(page); + + await page.getByTitle('Add comment to src/main.ts:5').click(); + const draft = page.getByPlaceholder('Write a comment...'); + await draft.fill('half-written'); + + await backend.updatePR(PR42, { + ReplaceInDiff: ["console.log('hello');", "console.log('hello, world');"], + Event: 'pull_request.synchronize', + Changes: ['pr', 'head'], + Actor: 'alice', + Summary: 'alice pushed new commits', + }); + + await expect(toast(page)).toContainText('alice pushed new commits'); + await expect(toast(page)).toContainText('It will refresh once you finish your comment.'); + await expect(diffRow(page, "console.log('hello');")).toBeVisible(); + await expect(draft).toHaveValue('half-written'); + + await page.getByRole('button', { name: 'Cancel' }).click(); + await expect(diffRow(page, "console.log('hello, world');")).toBeVisible(); + await expect(diffRow(page, "console.log('hello');")).toHaveCount(0); + await expect(toast(page)).toHaveText('Refreshed with the latest changes'); + }); + + test('a change to the list rows alone leaves the review be', async ({ page, backend }) => { + await openReview(page); + await connected(page); + + await backend.updatePR(PR42, { Event: 'sections', Changes: ['sections'] }); + // Longer than a batch collects for, so the next update can't join it. + await page.waitForTimeout(1000); + await backend.updatePR(PR42, { + Comment: { author: 'dave', body: 'One more thing.' }, + Actor: 'dave', + Summary: 'dave commented', + }); + + await expect(toast(page)).toHaveText('dave commented'); + expect(await backend.calls('GetPR')).toHaveLength(2); + }); + + test('the list refreshes a changed row and highlights it', async ({ page, backend }) => { + await page.goto('/'); + await expect(page.getByText('Add greeting helper')).toBeVisible(); + await connected(page); + + await backend.updatePR(PR42, { + Title: 'Add a greeting helper', + Event: 'pull_request.edited', + Changes: ['pr', 'metadata'], + Actor: 'alice', + Summary: 'alice edited the title', + }); + + await expect(toast(page)).toHaveText('widgets#42: alice edited the title'); + const row = page.locator('.crs-row').filter({ hasText: 'Add a greeting helper' }); + await expect(row).toHaveClass(/crs-row-updated/); + await expect(page.getByText('Loading reviews…')).toHaveCount(0); + expect(await backend.calls('GetAllReviews')).toHaveLength(2); + + // The highlight is a moment's. + await expect(row).not.toHaveClass(/crs-row-updated/); + }); +}); diff --git a/bun_client/frontend/src/App.css b/bun_client/frontend/src/App.css index 99d1b926..5250494a 100644 --- a/bun_client/frontend/src/App.css +++ b/bun_client/frontend/src/App.css @@ -387,6 +387,28 @@ details[open] > summary.crs-sidebar-label::after { background: var(--bg-secondary); } +/* A row the server just reported a change to: a wash of accent that fades. */ +.crs-row-updated { + animation: crsRowUpdated 2.5s ease-out; +} + +@keyframes crsRowUpdated { + 0%, + 30% { + background: var(--accent-dim); + } + 100% { + background: transparent; + } +} + +@media (prefers-reduced-motion: reduce) { + .crs-row-updated { + animation: none; + background: var(--accent-dim); + } +} + .crs-row-link { display: flex; align-items: flex-start; diff --git a/bun_client/frontend/src/App.tsx b/bun_client/frontend/src/App.tsx index 27dd5161..766e70cf 100644 --- a/bun_client/frontend/src/App.tsx +++ b/bun_client/frontend/src/App.tsx @@ -355,6 +355,7 @@ function App() { theme={theme} reviewLocation={reviewLocation} onThemeChange={setTheme} + onToast={showToast} /> )} {view === 'REVIEW' && currentPR && ( diff --git a/bun_client/frontend/src/components/PRList.tsx b/bun_client/frontend/src/components/PRList.tsx index 49a0b61b..1938ca11 100644 --- a/bun_client/frontend/src/components/PRList.tsx +++ b/bun_client/frontend/src/components/PRList.tsx @@ -1,7 +1,8 @@ -import { useEffect, useMemo, useState, type ReactNode } from 'react'; +import { useEffect, useMemo, useRef, useState, type ReactNode } from 'react'; import { listAIFeatures, rpcCall } from '../api'; import { reportFeatures, type AIFeatureInfo } from '../ai_utils'; import { useIsMobile } from '../hooks/useMediaQuery'; +import { usePRUpdates } from '../hooks/usePRUpdates'; import { Button, Input, @@ -13,6 +14,7 @@ import { ReviewLocation, type ReviewEaseDisplay, type ReviewEaseLevel, + type StatusVariant, } from '../design'; import { PR_STATES, @@ -33,6 +35,7 @@ import { } from '../pr_list_utils'; import type { Facet, PRState, ReviewItem } from '../pr_list_utils'; import { teamChips } from '../team_utils'; +import { CHANGE_SECTIONS, isUpdateFor, prUpdateMessage, type PRUpdate } from '../server_events'; interface PRListProps { onOpenReview: (owner: string, repo: string, number: number) => void; @@ -41,6 +44,8 @@ interface PRListProps { theme: Theme; reviewLocation: ReviewLocation; onThemeChange: (theme: Theme) => void; + /** Raises a transient notification, e.g. that a listed PR just changed. */ + onToast?: (message: string, variant?: StatusVariant) => void; } interface GetReviewsResponse { @@ -48,6 +53,13 @@ interface GetReviewsResponse { items: ReviewItem[]; } +/** How long a row the server just reported a change to stays highlighted. */ +const UPDATED_HIGHLIGHT_MS = 2500; + +/** Names a PR the same however its owner and repository are capitalized. */ +const prKeyOf = (owner: string, repo: string, number: number) => + `${owner}/${repo}#${number}`.toLowerCase(); + /** The sidebar's lifecycle buckets, plus the catch-all. */ type StateFilter = PRState | 'all'; @@ -238,6 +250,7 @@ export default function PRList({ theme: _theme, reviewLocation, onThemeChange: _onThemeChange, + onToast, }: PRListProps) { const isMobile = useIsMobile(); const [items, setItems] = useState([]); @@ -277,6 +290,54 @@ export default function PRList({ loadList(); }, []); + // Rows a webhook delivery just changed, by prKeyOf, highlighted for a + // moment so the eye finds what moved. + const [updatedRows, setUpdatedRows] = useState>(new Set()); + const highlightTimer = useRef | undefined>(undefined); + useEffect(() => () => clearTimeout(highlightTimer.current), []); + // Bumped per live refresh, so a slow one can't land over a newer one. + const refreshSeq = useRef(0); + + // A webhook delivery changed a PR, or the sections listing it: fetch the + // list again in place — no spinner, filters and collapsed sections kept — + // and say what happened to the PRs it shows. + const refreshLive = async (updates: PRUpdate[]) => { + const seq = ++refreshSeq.current; + let loaded: ReviewItem[]; + try { + const res = await rpcCall('RPCHandler.GetAllReviews', [{}]); + loaded = res.items || []; + } catch (e) { + console.error('Could not refresh the list after the server reported a change:', e); + return; + } + if (seq !== refreshSeq.current) return; + // A PR it showed, or shows now: one that just left a section was in it. + const shown = updates.filter(update => + [...items, ...loaded].some(item => + isUpdateFor(update, item.owner, item.repo, item.number) + ) + ); + setItems(loaded); + setLoadError(false); + if (shown.length === 0) return; + + setUpdatedRows(new Set(shown.map(u => prKeyOf(u.owner, u.repo, u.number)))); + clearTimeout(highlightTimer.current); + highlightTimer.current = setTimeout(() => setUpdatedRows(new Set()), UPDATED_HIGHLIGHT_MS); + const message = prUpdateMessage(shown, true); + if (message) onToast?.(message, 'info'); + }; + + // Only a section change can bring in a PR the list doesn't show yet; any + // other is about a PR the server holds but no section lists. + usePRUpdates( + update => + update.changes.includes(CHANGE_SECTIONS) || + items.some(item => isUpdateFor(update, item.owner, item.repo, item.number)), + refreshLive + ); + // The AI features are optional: none are enabled by default, and an older // server has no AI RPCs at all. Either way the rows simply show no AI // button. @@ -703,6 +764,9 @@ export default function PRList({ +
(next: T): (prev: T) => T { + return prev => (JSON.stringify(prev) === JSON.stringify(next) ? prev : next); +} + interface ReviewProps { owner: string; repo: string; @@ -244,6 +257,7 @@ export default function Review({ useEffect(() => { loadPR(); prKey.current = prRef; + deferredUpdates.current = null; setOpenAIFeature(null); setAIOutputs({}); loadAIFeatures(); @@ -342,11 +356,19 @@ export default function Review({ onToast: showToast, }); + // The diff as the server last sent it, before any hunk was expanded into + // it: what a live refresh compares a new payload's diff against. + const serverDiff = useRef(''); + const applyServerDiff = (next: string) => { + serverDiff.current = next; + setDiff(next); + setExpandedLineIndices(new Set()); + }; + // Apply a PR response payload to component state. const applyPRResponse = (res: PRResponse) => { setContent(res.content || ''); - setDiff(res.diff || ''); - setExpandedLineIndices(new Set()); + applyServerDiff(res.diff || ''); setComments(res.comments || []); setOutdatedComments(res.outdated_comments || []); setReviews(res.reviews || []); @@ -399,6 +421,77 @@ export default function Review({ } }; + // A comment being written is anchored to a row of the diff on screen, so a + // live refresh that brings in a new diff waits until it is done. + const composing = activeLineIndex !== null || editingCommentId !== null; + const composingRef = useRef(composing); + composingRef.current = composing; + // The updates a live refresh held back for a comment being written. + const deferredUpdates = useRef(null); + // Bumped per live refresh, so a slow one can't land over a newer one. + const refreshSeq = useRef(0); + + // A webhook delivery changed this PR (see server_events.ts): fetch it again + // — from the server's caches, which the delivery has just updated — and + // take in what changed without the loading state a sync shows. Open + // threads, expanded hunks, the comment being written and the feedback + // being typed are left as they are; only a new diff resets the expanded + // hunks, as a sync does. A toast says what happened. + const refreshLive = async (updates: PRUpdate[], held = false) => { + const key = prKey.current; + const seq = ++refreshSeq.current; + let res: PRResponse; + try { + res = await rpcCall('RPCHandler.GetPR', [ + { Owner: owner, Repo: repo, Number: number }, + ]); + } catch (e) { + console.error('Could not refresh the PR after the server reported a change:', e); + return; + } + if (prKey.current !== key || refreshSeq.current !== seq) return; + const nextDiff = res.diff || ''; + const diffChanged = nextDiff !== serverDiff.current; + if (diffChanged && composingRef.current) { + const holding = deferredUpdates.current !== null; + deferredUpdates.current = [...(deferredUpdates.current ?? []), ...updates]; + if (!holding) { + const message = prUpdateMessage(updates) ?? 'This PR was updated'; + showToast(`${message}\nIt will refresh once you finish your comment.`, 'info'); + } + return; + } + if (diffChanged) applyServerDiff(nextDiff); + // Most deliveries change one thing (a check run, the CI line), so what + // came back unchanged keeps its identity and its memos stand. + setContent(res.content || ''); + setComments(unlessSame(res.comments || [])); + setOutdatedComments(unlessSame(res.outdated_comments || [])); + setReviews(unlessSame(res.reviews || [])); + setMetadata(unlessSame(res.metadata || null)); + // Comments can change what the AI features cover, and a push re-runs + // the plugins. + loadAIOutputs(); + if (diffChanged || updates.some(u => u.changes.includes(CHANGE_HEAD))) { + loadPluginOutputs(); + } + const message = held ? 'Refreshed with the latest changes' : prUpdateMessage(updates); + if (message) showToast(message, 'info'); + }; + + usePRUpdates( + update => isUpdateFor(update, owner, repo, number) && changesPR(update), + updates => refreshLive(updates) + ); + + // Take in what was held back once the comment is posted or cancelled. + useEffect(() => { + const held = deferredUpdates.current; + if (composing || !held) return; + deferredUpdates.current = null; + refreshLive(held, true); + }, [composing]); + // Expand a hunk by fetching extra context lines from the server and // splicing them into the current diff text. const handleExpandHunk = async ( @@ -518,8 +611,7 @@ export default function Review({ } const res = await rpcCall('RPCHandler.AddComment', [params]); setContent(res.content || ''); - setDiff(res.diff || ''); - setExpandedLineIndices(new Set()); + applyServerDiff(res.diff || ''); const prevIds = new Set(comments.map(c => c.id)); const newComments = (res.comments || []).filter(c => !prevIds.has(c.id)); if (newComments.length > 0) { @@ -622,8 +714,7 @@ export default function Review({ }, ]); setContent(res.content || ''); - setDiff(res.diff || ''); - setExpandedLineIndices(new Set()); + applyServerDiff(res.diff || ''); setComments(res.comments || []); setOutdatedComments(res.outdated_comments || []); setReviews(res.reviews || []); diff --git a/bun_client/frontend/src/hooks/usePRUpdates.ts b/bun_client/frontend/src/hooks/usePRUpdates.ts new file mode 100644 index 00000000..c734911d --- /dev/null +++ b/bun_client/frontend/src/hooks/usePRUpdates.ts @@ -0,0 +1,29 @@ +import { useEffect, useRef } from 'react'; +import { onPRUpdates, serverEvents, type PRUpdate, type ServerEvents } from '../server_events'; + +/** + * Calls `onUpdates` with each batch of the server's PR updates (a webhook + * delivery changing a PR) that `accept` lets through. Both are read fresh on + * every update, so they can close over the latest state without the stream + * being re-subscribed each render. + */ +export function usePRUpdates( + accept: (update: PRUpdate) => boolean, + onUpdates: (updates: PRUpdate[]) => void, + events: ServerEvents = serverEvents +) { + const acceptRef = useRef(accept); + const onUpdatesRef = useRef(onUpdates); + acceptRef.current = accept; + onUpdatesRef.current = onUpdates; + + useEffect( + () => + onPRUpdates( + events, + update => acceptRef.current(update), + updates => onUpdatesRef.current(updates) + ), + [events] + ); +} diff --git a/bun_client/frontend/src/server_events.test.tsx b/bun_client/frontend/src/server_events.test.tsx index f76e93e7..06890a88 100644 --- a/bun_client/frontend/src/server_events.test.tsx +++ b/bun_client/frontend/src/server_events.test.tsx @@ -2,14 +2,22 @@ import { describe, expect, test } from 'bun:test'; import { renderToStaticMarkup } from 'react-dom/server'; import ServerStatus from './components/ServerStatus'; import { + changesPR, HEARTBEAT_EVENT, heartbeatDeadlineMs, heartbeatReducer, INITIAL_HEARTBEAT_STATE, + isUpdateFor, + onPRUpdates, parseHeartbeat, + parsePRUpdate, + PR_UPDATED_EVENT, + prUpdateMessage, ServerEvents, type EventStream, type Heartbeat, + type PRUpdate, + type Timers, } from './server_events'; const BEAT: Heartbeat = { @@ -203,3 +211,177 @@ describe('ServerStatus', () => { expect(compact).not.toContain('>Server not responding'); }); }); + +const UPDATE: PRUpdate = { + owner: 'C-Hipple', + repo: 'code-review-server', + number: 303, + event: 'pull_request_review.submitted', + changes: ['reviews', 'metadata'], + head_sha: 'abc123', + actor: 'alice', + own: false, + summary: 'alice approved', +}; + +describe('parsePRUpdate', () => { + test('reads a PR update', () => { + expect(parsePRUpdate({ ...UPDATE, time: '2026-10-25T12:00:00Z' })).toEqual(UPDATE); + }); + + test('fills in what a quiet update leaves out', () => { + expect( + parsePRUpdate({ + owner: 'a', + repo: 'b', + number: 1, + event: 'sections', + changes: ['sections'], + }) + ).toEqual({ + owner: 'a', + repo: 'b', + number: 1, + event: 'sections', + changes: ['sections'], + head_sha: undefined, + actor: undefined, + own: false, + summary: undefined, + }); + }); + + test('rejects anything else', () => { + expect(parsePRUpdate(null)).toBeNull(); + expect(parsePRUpdate({ ...UPDATE, number: '303' })).toBeNull(); + expect(parsePRUpdate({ ...UPDATE, changes: 'reviews' })).toBeNull(); + expect(parsePRUpdate({ owner: 'a' })).toBeNull(); + }); +}); + +describe('isUpdateFor', () => { + test('matches the PR however it is capitalized', () => { + expect(isUpdateFor(UPDATE, 'c-hipple', 'Code-Review-Server', 303)).toBe(true); + expect(isUpdateFor(UPDATE, 'C-Hipple', 'code-review-server', 302)).toBe(false); + expect(isUpdateFor(UPDATE, 'C-Hipple', 'gtdbot', 303)).toBe(false); + }); +}); + +describe('changesPR', () => { + test('is false for a change to the list rows alone', () => { + expect(changesPR(UPDATE)).toBe(true); + expect(changesPR({ ...UPDATE, changes: ['sections'] })).toBe(false); + expect(changesPR({ ...UPDATE, changes: ['sections', 'head'] })).toBe(true); + }); +}); + +describe('prUpdateMessage', () => { + test('says what happened, naming the PR when asked', () => { + expect(prUpdateMessage([UPDATE])).toBe('alice approved'); + expect(prUpdateMessage([UPDATE], true)).toBe('code-review-server#303: alice approved'); + }); + + test("leaves out quiet updates and the user's own", () => { + expect(prUpdateMessage([{ ...UPDATE, summary: undefined }])).toBeNull(); + expect(prUpdateMessage([{ ...UPDATE, own: true }])).toBeNull(); + expect( + prUpdateMessage([ + { ...UPDATE, own: true }, + { ...UPDATE, summary: 'Checks passed' }, + ]) + ).toBe('Checks passed'); + }); + + test('says a repeat once and sums up a long burst', () => { + const said = (summary: string) => ({ ...UPDATE, summary }); + expect(prUpdateMessage([said('Checks passed'), said('Checks passed')])).toBe( + 'Checks passed' + ); + expect(prUpdateMessage(['a', 'b', 'c', 'd', 'e'].map(said))).toBe('c\nd\ne\n…and 2 more'); + }); +}); + +/** Timers a test fires by hand. */ +function manualTimers() { + const pending = new Map void; ms: number }>(); + let next = 0; + const timers: Timers = { + set: (fn, ms) => { + pending.set(++next, { fn, ms }); + return next; + }, + clear: handle => pending.delete(handle as number), + }; + const fire = () => { + const due = [...pending.values()]; + pending.clear(); + for (const { fn } of due) fn(); + }; + return { timers, pending, fire }; +} + +describe('onPRUpdates', () => { + test('batches a burst into one flush, bounded by the first update', () => { + const { events, opened } = hub(); + const { timers, pending, fire } = manualTimers(); + const flushed: PRUpdate[][] = []; + onPRUpdates( + events, + () => true, + updates => flushed.push(updates), + 400, + timers + ); + + opened[0].emit(PR_UPDATED_EVENT, JSON.stringify(UPDATE)); + opened[0].emit(PR_UPDATED_EVENT, JSON.stringify({ ...UPDATE, summary: 'Checks passed' })); + // The second joined the first's batch rather than putting it off. + expect(pending.size).toBe(1); + expect([...pending.values()][0].ms).toBe(400); + expect(flushed).toEqual([]); + + fire(); + expect(flushed.map(batch => batch.map(u => u.summary))).toEqual([ + ['alice approved', 'Checks passed'], + ]); + + opened[0].emit(PR_UPDATED_EVENT, JSON.stringify(UPDATE)); + fire(); + expect(flushed).toHaveLength(2); + }); + + test('ignores what it does not accept, and anything that is not an update', () => { + const { events, opened } = hub(); + const { timers, pending } = manualTimers(); + onPRUpdates( + events, + update => update.number === 1, + () => {}, + 400, + timers + ); + + opened[0].emit(PR_UPDATED_EVENT, JSON.stringify(UPDATE)); + opened[0].emit(PR_UPDATED_EVENT, '{"owner":"a"}'); + expect(pending.size).toBe(0); + }); + + test('drops a pending batch on unsubscribe', () => { + const { events, opened } = hub(); + const { timers, pending } = manualTimers(); + let flushed = 0; + const off = onPRUpdates( + events, + () => true, + () => flushed++, + 400, + timers + ); + + opened[0].emit(PR_UPDATED_EVENT, JSON.stringify(UPDATE)); + off(); + expect(pending.size).toBe(0); + expect(flushed).toBe(0); + expect(opened[0].closed).toBe(true); + }); +}); diff --git a/bun_client/frontend/src/server_events.ts b/bun_client/frontend/src/server_events.ts index 4405f053..d91c9285 100644 --- a/bun_client/frontend/src/server_events.ts +++ b/bun_client/frontend/src/server_events.ts @@ -75,6 +75,153 @@ export function heartbeatReducer(state: HeartbeatState, action: HeartbeatAction) } } +/** + * Sent when a GitHub webhook delivery changed what the server holds for a PR: + * a page showing it fetches it again (GetPR, GetAllReviews) to pick the change + * up, and says what happened. + */ +export const PR_UPDATED_EVENT = 'Server.PRUpdated'; + +/** `changes` when the PR's head or base moved: its diff may be new. */ +export const CHANGE_HEAD = 'head'; +/** `changes` when only the review list's rows for the PR changed. */ +export const CHANGE_SECTIONS = 'sections'; + +export interface PRUpdate { + owner: string; + /** The short repository name, as GetPR takes it. */ + repo: string; + number: number; + /** The delivery's event and action ("pull_request_review.submitted"), or "sections". */ + event: string; + /** What was rewritten: "head", "pr", "reviews", "comments", "ci_status", "sections", ... */ + changes: string[]; + head_sha?: string; + /** Who did it; absent for CI. */ + actor?: string; + /** The change is the configured user's own. */ + own: boolean; + /** What happened in a few words ("alice approved"); absent when not worth announcing. */ + summary?: string; +} + +export function parsePRUpdate(data: unknown): PRUpdate | null { + if (typeof data !== 'object' || data === null) return null; + const { owner, repo, number, event, changes, head_sha, actor, own, summary } = data as Record< + string, + unknown + >; + if ( + typeof owner !== 'string' || + typeof repo !== 'string' || + typeof number !== 'number' || + !Array.isArray(changes) + ) { + return null; + } + return { + owner, + repo, + number, + event: typeof event === 'string' ? event : '', + changes: changes.filter((c): c is string => typeof c === 'string'), + head_sha: typeof head_sha === 'string' ? head_sha : undefined, + actor: typeof actor === 'string' ? actor : undefined, + own: own === true, + summary: typeof summary === 'string' && summary !== '' ? summary : undefined, + }; +} + +/** + * Whether `update` is about the PR owner/repo#number. GitHub and the config + * can spell an owner or repository in different cases. + */ +export function isUpdateFor(update: PRUpdate, owner: string, repo: string, number: number) { + return ( + update.number === number && + update.owner.toLowerCase() === owner.toLowerCase() && + update.repo.toLowerCase() === repo.toLowerCase() + ); +} + +/** Whether `update` changed the PR itself, not just the review list's rows for it. */ +export function changesPR(update: PRUpdate): boolean { + return update.changes.some(change => change !== CHANGE_SECTIONS); +} + +/** Lines a toast shows before it sums the rest up. */ +const SHOWN_UPDATES = 3; + +/** + * What a toast says about a batch of updates: one line per announced change, + * the newest few, each naming its PR when `withPR`. Null when none is worth + * announcing — the rest only refresh the page. Changes the user made + * themselves aren't announced: they know. + */ +export function prUpdateMessage(updates: PRUpdate[], withPR = false): string | null { + const lines: string[] = []; + for (const update of updates) { + if (!update.summary || update.own) continue; + const line = withPR ? `${update.repo}#${update.number}: ${update.summary}` : update.summary; + // A burst can say the same thing twice (a check re-run that passes again). + if (lines[lines.length - 1] !== line) lines.push(line); + } + if (lines.length === 0) return null; + const shown = lines.slice(-SHOWN_UPDATES); + const hidden = lines.length - shown.length; + return hidden > 0 ? `${shown.join('\n')}\n…and ${hidden} more` : shown.join('\n'); +} + +/** How long a batch of updates collects before the page acts on it. */ +export const PR_UPDATE_BATCH_MS = 400; + +export interface Timers { + set: (fn: () => void, ms: number) => unknown; + clear: (handle: unknown) => void; +} + +const realTimers: Timers = { + set: (fn, ms) => setTimeout(fn, ms), + clear: handle => clearTimeout(handle as ReturnType), +}; + +/** + * Calls `flush` with each batch of the PR updates `accept` lets through. A + * delivery seldom comes alone — a push sets off one per check, a review one + * per comment, then the section change — so the first update opens a batch, + * and whatever arrives in the next `delayMs` joins it rather than refreshing + * the page again. The batch never waits longer than that, however busy it + * gets. Returns the unsubscribe. + */ +export function onPRUpdates( + events: ServerEvents, + accept: (update: PRUpdate) => boolean, + flush: (updates: PRUpdate[]) => void, + delayMs: number = PR_UPDATE_BATCH_MS, + timers: Timers = realTimers +): () => void { + let batch: PRUpdate[] = []; + let timer: unknown = null; + const off = events.on(PR_UPDATED_EVENT, data => { + const update = parsePRUpdate(data); + if (!update || !accept(update)) return; + batch.push(update); + if (timer !== null) return; + timer = timers.set(() => { + const updates = batch; + batch = []; + timer = null; + flush(updates); + }, delayMs); + }); + return () => { + off(); + if (timer !== null) timers.clear(timer); + timer = null; + batch = []; + }; +} + type MessageListener = (event: MessageEvent) => void; /** The parts of EventSource ServerEvents uses, so tests can stand in for it. */ diff --git a/docs/building_clients.md b/docs/building_clients.md index 49230de4..99803cd8 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 a `Server.PRUpdated` whenever a GitHub webhook delivery changes a PR (refetch it with `GetPR` to show the change), 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). 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/protocol.md b/docs/protocol.md index 334a1b71..c688478b 100644 --- a/docs/protocol.md +++ b/docs/protocol.md @@ -46,6 +46,31 @@ 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.PRUpdated` + +Sent when a [GitHub webhook](webhooks.md) delivery has changed what the server holds for a PR: once the delivery is applied to the caches, for each PR it rewrote anything for, and again when the check that follows rewrites the PR's [section rows](webhooks.md#sections-follow-deliveries). Nothing is sent for a delivery about a PR the server doesn't hold, or one that found nothing cached to patch. Without `[Webhooks]` none is ever sent. + +The notification says what changed, not what it changed to: a client showing the PR fetches it again with `GetPR` (or the list with `GetAllReviews`), which the delivery's changes are already cached for, so the refetch never waits on GitHub. + +```json +{"method": "Server.PRUpdated", "params": [{"owner": "C-Hipple", "repo": "code-review-server", "number": 303, "event": "pull_request_review.submitted", "changes": ["reviews", "requested_reviewers", "metadata"], "head_sha": "9f2c1e7", "actor": "alice", "own": false, "summary": "alice approved", "time": "2026-10-26T14:02:11Z"}], "id": null} +``` + +| Field | Type | Description | +|------------|----------|-------------| +| `owner` | string | The PR's owner, as GitHub spells it (compare case-insensitively with the config's spelling) | +| `repo` | string | The repository's short name, as `GetPR` takes it | +| `number` | int | The PR number | +| `event` | string | The delivery's event and action (`pull_request.synchronize`, `issue_comment.created`, `check_run.completed`, `status.success`, ...), or `sections` for a change to the PR's section rows | +| `changes` | string[] | What was rewritten: `head` (the head or base moved and the PR was fetched again in full, diff included), `pr` (title, description, state, draft, labels, assignees, review requests, and the list rows built from them), the cache fields a delivery patched (`reviews`, `comments`, `review_threads`, `ci_status`, `metadata`, `requested_reviewers`, `team_reviews`), or `sections` (the PR joined, left or had its rows rewritten in a section). Never empty | +| `head_sha` | string | The PR's head as the delivery saw it; omitted when the delivery carries no PR (a conversation comment, a check run, a commit status) | +| `actor` | string | Who did it, from the delivery's sender; omitted for CI and `sections` | +| `own` | bool | `actor` is the configured `GithubUsername`: the user's own change, made in a client or on GitHub | +| `summary` | string | What happened in a few words, without naming the PR: `alice approved`, `bob commented on server/notify.go`, `alice pushed new commits`, `Checks failed: build, lint`. Omitted for a change not worth announcing — a check starting or another one failing once CI already has, a row rewritten | +| `time` | string | The server's clock when it was sent (RFC 3339) | + +Deliveries come in bursts — a push sets off one per check, then the section change — so expect several of these close together for one PR, and refetch once per burst rather than once per notification. + --- ## Methods @@ -69,7 +94,7 @@ A simple health check / test method. ### `RPCHandler.Subscribe` -Turns on [server notifications](#server-notifications) for this connection: a `Server.Heartbeat` every interval, and any event the server sends. Calling it again changes the interval. +Turns on [server notifications](#server-notifications) for this connection: a `Server.Heartbeat` every interval, and any event the server sends — a [`Server.PRUpdated`](#serverprupdated) for each PR a webhook delivery changes. Calling it again changes the interval. **Arguments** (`SubscribeArgs`): | Field | Type | Required | Description | diff --git a/docs/web_client_features.md b/docs/web_client_features.md index c0811efe..93eb15cb 100644 --- a/docs/web_client_features.md +++ b/docs/web_client_features.md @@ -129,7 +129,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.PRUpdated`), 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 @@ -421,6 +421,8 @@ markdown is not just markdown: The toolbar's Sync button calls `SyncPR` and reports the result through a transient toast, distinguishing "new commits or comments pulled in" from "already up to date" using the reply's `updated` flag. Plugin outputs are reloaded at the same time. +With [webhooks](webhooks.md) on, a review usually needn't be synced: changes arrive by +themselves (§3.18). ### 3.9 Plugins @@ -645,6 +647,35 @@ Both run and render a report the same way (`useAIReport`, `AIReportView`): diagram inline in its card. - **Any other feature** renders its markdown `body` with `PluginBodyView`. +### 3.18 Live updates from webhooks (`usePRUpdates.ts`, `server_events.ts`) + +With [`[Webhooks]`](webhooks.md) configured, the server sends a +[`Server.PRUpdated`](protocol.md#serverprupdated) for each PR a delivery changes, +and the bridge relays it over `/api/events` like the heartbeat. `onPRUpdates` +collects a burst — a push sets off a delivery per check, then the section change — +into one batch: the first update opens it and the rest join it for 400 ms, so the +page refreshes once per burst and never waits longer than that. + +- **Review** (`Review.tsx`, `refreshLive`): an update for the open PR, other than one + that only touched the list's rows (`sections`), re-calls `GetPR` — served from the + caches the delivery just wrote — and takes in the comments, reviews, metadata and, + when it changed, the diff, without the loading state a sync shows. Open threads, + the comment being written and the feedback being typed are left alone, and + expanded hunk context stays unless the diff itself changed. A new diff while an + inline comment is open or a comment is being edited would move the row it is + anchored to, so it waits: a toast says the PR changed and will refresh once the + comment is done, and closing the form brings it in. A push also reloads the + plugin outputs; every refresh reloads the AI outputs' counts. +- **List** (`PRList.tsx`): any update re-calls `GetAllReviews` in place — filters, + collapsed sections and scroll kept, no spinner, no plugin warm-up — and the rows + of the PRs it named flash with the accent colour for a moment (`.crs-row-updated`, + a steady tint under `prefers-reduced-motion`). +- **Messages** (`prUpdateMessage`): the toast shows each update's `summary` (`alice + approved`, `Checks passed`), prefixed with `repo#number` on the list, the newest + three of a burst with a count of the rest. Updates without a summary — a check + starting, a row rewritten — and the user's own changes (`own`) refresh silently; + the list says nothing about PRs it doesn't show. + ## 4. Protocol methods used — and not used Called by the current client: diff --git a/docs/webhooks.md b/docs/webhooks.md index 06c92f6c..73d84a85 100644 --- a/docs/webhooks.md +++ b/docs/webhooks.md @@ -105,6 +105,10 @@ What "matches" means is what the workflow's own run would decide: for a reposito This happens only while webhooks are trusted for the PR, and only when its caches hold its current head, so it never asks GitHub for anything. A delivery that lands while a cycle is refreshing everything is checked as soon as that cycle finishes. A delivery with no PR in it (a conversation comment, a check run, a commit status) is judged against the PR as the last listing or delivery showed it. If a cycle is writing sections when a delivery lands, the check waits for it to finish, so a cycle working from an older listing can't undo it. A burst of deliveries about one PR — a push sets off a check run delivery per check — is checked once. +## Clients Follow Deliveries + +The server also tells a subscribed client about every PR a delivery changed — a [`Server.PRUpdated`](protocol.md#serverprupdated) notification naming the PR, what was rewritten and, in a few words, what happened — and again when the check above rewrites the PR's section rows. The web client takes it from there: an open review fetches the PR again from the caches and takes in the change where it stands, keeping the reader's place, and a toast says what happened (*alice approved*, *Checks failed: build*); the review list refreshes the PR's row and highlights it for a moment. CI is announced only as it settles — passing, or failing where it wasn't — though the CI line updates on every delivery; your own changes refresh without a toast. See [Live updates from webhooks](web_client_features.md) (§3.18). + ## When the Server Still Polls Every PR is still polled, as without webhooks: diff --git a/main.go b/main.go index d66e64e3..ad2e75aa 100644 --- a/main.go +++ b/main.go @@ -106,6 +106,9 @@ func main() { // workflow-only process exits when its cycle ends and would kill the // hooks mid-run. workflows.SetPRUpdatedHook(server.WarmPRAnalysis) + // Tell a subscribed client — the web client's bridge — about each PR + // a webhook delivery changes, so a page showing it can refresh. + workflows.SetPRChangedHook(server.NotifyPRChanged) go func() { slog.Info("Starting pprof server", "addr", "localhost:6061") diff --git a/server/notify.go b/server/notify.go index 58945f1a..e1a48e8a 100644 --- a/server/notify.go +++ b/server/notify.go @@ -1,10 +1,13 @@ package server import ( + "crs/config" + "crs/workflows" "encoding/json" "fmt" "io" "log/slog" + "strings" "sync" "time" ) @@ -18,13 +21,17 @@ 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. +// There are two so far: the heartbeat, sent every interval while a client is +// subscribed, and Server.PRUpdated, sent when a GitHub webhook delivery has +// changed what the server holds for a PR. Notifier.Notify is the seam the next +// event goes through. // HeartbeatMethod is the method a heartbeat notification names. const HeartbeatMethod = "Server.Heartbeat" +// PRUpdatedMethod is the method a PR-updated notification names. +const PRUpdatedMethod = "Server.PRUpdated" + const ( defaultHeartbeatInterval = 10 * time.Second minHeartbeatInterval = time.Second @@ -62,6 +69,67 @@ type Heartbeat struct { IntervalSeconds int `json:"interval_seconds"` } +// PRUpdated is a PR-updated notification's params: a webhook delivery changed +// what the server holds for the PR, so a client showing it can fetch it again +// (GetPR, GetAllReviews) and find the change there. +type PRUpdated struct { + // Owner, Repo and Number name the PR. Repo is the short name, as GetPR + // takes it; Owner is spelled as GitHub spells it, which may differ in case + // from the config. + Owner string `json:"owner"` + Repo string `json:"repo"` + Number int `json:"number"` + // Event is the delivery's event and action + // ("pull_request_review.submitted"), or "sections" when what changed is + // the review list's rows for the PR rather than the PR. + Event string `json:"event"` + // Changes names what was rewritten: "head" (the head or base moved; the PR + // was fetched again in full, diff included), "pr" (the PR's title, + // description, state, labels, review requests), a cache field ("reviews", + // "comments", "review_threads", "ci_status", "metadata", ...), or + // "sections". + Changes []string `json:"changes"` + // HeadSHA is the PR's head as the delivery saw it; empty when the delivery + // carries no PR. + HeadSHA string `json:"head_sha,omitempty"` + // Actor is who did it; empty for CI. + Actor string `json:"actor,omitempty"` + // Own is true when Actor is the configured GithubUsername: the change is + // the user's own, made here or on GitHub. + Own bool `json:"own"` + // Summary says what happened in a few words ("alice approved", "Checks + // failed: build"), without naming the PR; empty for a change not worth + // announcing (a check starting, a row rewritten). + Summary string `json:"summary,omitempty"` + // Time is the server's clock when it was sent (RFC 3339). + Time string `json:"time"` +} + +// NotifyPRChanged tells a subscribed client that a webhook delivery changed a +// PR. It is the hook the workflow layer is wired to (see +// workflows.SetPRChangedHook). +func NotifyPRChanged(c workflows.PRChange) { + if notifier == nil { + return + } + notifier.Notify(PRUpdatedMethod, prUpdated(c, config.C().GithubUsername, time.Now())) +} + +func prUpdated(c workflows.PRChange, me string, now time.Time) PRUpdated { + return PRUpdated{ + Owner: c.Key.Owner, + Repo: c.Key.Repo, + Number: c.Key.Number, + Event: c.Event, + Changes: c.Changes, + HeadSHA: c.HeadSHA, + Actor: c.Actor, + Own: c.Actor != "" && strings.EqualFold(c.Actor, me), + Summary: c.Summary, + Time: now.Format(time.RFC3339), + } +} + // notification is a JSON-RPC 1.0 notification as it goes on the wire. type notification struct { Method string `json:"method"` @@ -192,7 +260,8 @@ type SubscribeReply struct { } // Subscribe turns on server notifications for this connection: a -// Server.Heartbeat every interval, and whatever events the server sends. +// Server.Heartbeat every interval, and whatever events the server sends — a +// Server.PRUpdated for each PR a webhook delivery changes. // Calling it again changes the interval. func (h *RPCHandler) Subscribe(args *SubscribeArgs, reply *SubscribeReply) error { if notifier == nil { diff --git a/server/notify_test.go b/server/notify_test.go index f9b19135..0c1f5ab7 100644 --- a/server/notify_test.go +++ b/server/notify_test.go @@ -2,6 +2,8 @@ package server import ( "bufio" + "crs/config" + "crs/workflows" "encoding/json" "fmt" "io" @@ -359,3 +361,61 @@ func TestNotificationsShareTheConnectionWithReplies(t *testing.T) { } expectSilence(t, out, 50*time.Millisecond) } + +func TestNotifyPRChangedSendsAPRUpdated(t *testing.T) { + w, out := lines(t) + saved := notifier + notifier = newNotifier(w) + t.Cleanup(func() { + notifier.Unsubscribe() + notifier = saved + }) + config.SetC(config.Config{GithubUsername: "Me"}) + t.Cleanup(func() { config.SetC(config.Config{}) }) + + change := workflows.PRChange{ + Key: workflows.PRKey{Owner: "acme", Repo: "widgets", Number: 42}, + Event: "pull_request_review.submitted", + Changes: []string{"reviews", "metadata"}, + HeadSHA: "abc123", + Actor: "rev", + Summary: "rev approved", + } + // Nothing goes out before the client subscribes. + NotifyPRChanged(change) + expectSilence(t, out, 50*time.Millisecond) + + notifier.Subscribe(time.Hour) + NotifyPRChanged(change) + var n struct { + Method string `json:"method"` + Params []PRUpdated `json:"params"` + ID any `json:"id"` + } + line := nextLine(t, out, time.Second) + if err := json.Unmarshal([]byte(line), &n); err != nil { + t.Fatalf("not JSON: %s: %v", line, err) + } + if n.Method != PRUpdatedMethod || n.ID != nil || len(n.Params) != 1 { + t.Fatalf("notification = %s", line) + } + got := n.Params[0] + if got.Owner != "acme" || got.Repo != "widgets" || got.Number != 42 || got.Event != change.Event || + got.HeadSHA != "abc123" || got.Actor != "rev" || got.Summary != "rev approved" || got.Own || got.Time == "" || + strings.Join(got.Changes, ",") != "reviews,metadata" { + t.Errorf("params = %+v", got) + } + + // The user's own change says so, however the login is spelled. + change.Actor = "me" + NotifyPRChanged(change) + if line := nextLine(t, out, time.Second); !strings.Contains(line, `"own":true`) { + t.Errorf("the user's own change isn't marked own: %s", line) + } +} + +func TestPRUpdatedOwnNeedsAnActor(t *testing.T) { + if prUpdated(workflows.PRChange{}, "", time.Now()).Own { + t.Error("a change with no actor counted as the user's own when no login is configured") + } +} diff --git a/workflows/webhook_changes.go b/workflows/webhook_changes.go new file mode 100644 index 00000000..641c3548 --- /dev/null +++ b/workflows/webhook_changes.go @@ -0,0 +1,298 @@ +package workflows + +import ( + "fmt" + "log/slog" + "strings" + "sync/atomic" + + "github.com/google/go-github/v74/github" +) + +// Telling the clients what a webhook delivery changed. Once a delivery has +// been applied to the caches, each PR it rewrote anything for is handed to +// the registered PR-changed hook — server.NotifyPRChanged in server mode, +// which sends a subscribed client a Server.PRUpdated notification — so a page +// showing the PR can fetch it again and say what happened. So is a PR whose +// section rows the re-evaluation that follows rewrote (webhook_membership.go), +// with "sections" as the change. + +// PRChange is what one delivery changed about a PR we hold. +type PRChange struct { + Key PRKey + // Event is the delivery's event and action + // ("pull_request_review.submitted"), or "sections" for a re-evaluation. + Event string + // Changes names what was rewritten: a ChangeHead or ChangePR, the cache + // fields the delivery patched ("reviews", "comments", "review_threads", + // "ci_status", "metadata", ...), or ChangeSections. + Changes []string + // HeadSHA is the PR's head as the delivery saw it; empty when it carries + // no PR (a conversation comment, a check run, a commit status). + HeadSHA string + // Actor is who did it, from the delivery's sender; empty for CI. + Actor string + // Summary says what happened in a few words ("alice approved"), or is + // empty for a change not worth announcing: a check starting, a row + // rewritten. + Summary string +} + +const ( + // ChangeHead: the PR's head or base moved, so it was fetched again in + // full — diff, commits and everything else. + ChangeHead = "head" + // ChangePR: a pull_request delivery's PR object — title, description, + // state, draft, labels, assignees, review requests — and the review list + // rows built from it. + ChangePR = "pr" + // ChangeSections: the sections listing the PR, or their rows' details. + ChangeSections = "sections" +) + +// prChangedHook is called, on the webhook worker, for each PR a delivery +// changed. Nil when nothing is registered. +var prChangedHook atomic.Pointer[func(PRChange)] + +// SetPRChangedHook registers the callback told about each PR a webhook +// delivery changed. It runs on the goroutine applying deliveries, in delivery +// order, so it must not block. A nil fn clears it. +func SetPRChangedHook(fn func(PRChange)) { + if fn == nil { + prChangedHook.Store(nil) + return + } + prChangedHook.Store(&fn) +} + +func notifyPRChanged(c PRChange) { + hook := prChangedHook.Load() + if hook == nil || len(c.Changes) == 0 { + return + } + defer func() { + if p := recover(); p != nil { + slog.Error("Panic in the PR-changed hook", "pr", c.Key.Number, "repo", c.Key.Repo, "panic", p) + } + }() + (*hook)(c) +} + +// actorName is who a summary says did something. +func actorName(login string) string { + if login == "" { + return "Someone" + } + return login +} + +// pullRequestSummary says what a pull_request delivery reports; empty for an +// action not worth announcing (auto-merge settings, milestones, locks). +func pullRequestSummary(e *github.PullRequestEvent) string { + who := actorName(e.GetSender().GetLogin()) + pr := e.GetPullRequest() + switch e.GetAction() { + case "opened": + return who + " opened the PR" + case "reopened": + return who + " reopened the PR" + case "closed": + if pr.GetMerged() { + return who + " merged the PR" + } + return who + " closed the PR" + case "synchronize": + return who + " pushed new commits" + case "edited": + changes := e.GetChanges() + switch { + case changes.GetBase() != nil: + return fmt.Sprintf("%s changed the base branch to %s", who, pr.GetBase().GetRef()) + case changes.GetTitle() != nil: + return who + " edited the title" + case changes.GetBody() != nil: + return who + " edited the description" + } + return who + " edited the PR" + case "ready_for_review": + return who + " marked the PR ready for review" + case "converted_to_draft": + return who + " converted the PR to a draft" + case "review_requested": + return fmt.Sprintf("%s requested a review from %s", who, requestedName(e)) + case "review_request_removed": + return fmt.Sprintf("%s removed the review request for %s", who, requestedName(e)) + case "labeled": + return fmt.Sprintf("%s added the label %s", who, e.GetLabel().GetName()) + case "unlabeled": + return fmt.Sprintf("%s removed the label %s", who, e.GetLabel().GetName()) + case "assigned": + return fmt.Sprintf("%s assigned %s", who, e.GetAssignee().GetLogin()) + case "unassigned": + return fmt.Sprintf("%s unassigned %s", who, e.GetAssignee().GetLogin()) + } + return "" +} + +// requestedName is the reviewer or team a review-request delivery names. +func requestedName(e *github.PullRequestEvent) string { + if login := e.GetRequestedReviewer().GetLogin(); login != "" { + return login + } + if team := e.GetRequestedTeam(); team != nil { + if team.GetSlug() != "" { + return team.GetSlug() + } + return team.GetName() + } + return "a reviewer" +} + +// reviewSummary says what a pull_request_review delivery reports. +func reviewSummary(e *github.PullRequestReviewEvent) string { + who := actorName(e.GetSender().GetLogin()) + review := e.GetReview() + switch e.GetAction() { + case "submitted": + who = actorName(review.GetUser().GetLogin()) + switch strings.ToLower(review.GetState()) { + case "approved": + return who + " approved" + case "changes_requested": + return who + " requested changes" + } + return who + " reviewed" + case "edited": + return who + " edited a review" + case "dismissed": + return fmt.Sprintf("%s dismissed %s's review", who, actorName(review.GetUser().GetLogin())) + } + return "" +} + +// reviewCommentSummary says what a pull_request_review_comment delivery +// reports. +func reviewCommentSummary(e *github.PullRequestReviewCommentEvent) string { + who := actorName(e.GetSender().GetLogin()) + comment := e.GetComment() + where := "" + if path := comment.GetPath(); path != "" { + where = " on " + path + } + switch e.GetAction() { + case "created": + if comment.InReplyTo != nil { + return who + " replied" + where + } + return who + " commented" + where + case "edited": + return who + " edited a comment" + where + case "deleted": + return who + " deleted a comment" + where + } + return "" +} + +// reviewThreadSummary says what a pull_request_review_thread delivery reports. +func reviewThreadSummary(e *github.PullRequestReviewThreadEvent) string { + who := actorName(e.GetSender().GetLogin()) + where := "" + for _, c := range e.GetThread().Comments { + if c.GetPath() != "" { + where = " on " + c.GetPath() + break + } + } + switch e.GetAction() { + case "resolved": + return who + " resolved a thread" + where + case "unresolved": + return who + " reopened a thread" + where + } + return "" +} + +// issueCommentSummary says what an issue_comment delivery on a PR reports. +func issueCommentSummary(e *github.IssueCommentEvent) string { + who := actorName(e.GetSender().GetLogin()) + switch e.GetAction() { + case "created": + return who + " commented" + case "edited": + return who + " edited a comment" + case "deleted": + return who + " deleted a comment" + } + return "" +} + +// ciOutcome is a commit's CI in a word: "failure" once any check or status +// has failed, "pending" while any is still running, "success" when all have +// passed, and "" before any has reported. It also returns the failed ones' +// names. +func ciOutcome(status *github.CombinedStatus, checkRuns *github.ListCheckRunsResults) (string, []string) { + reported, pending := false, false + var failed []string + if status != nil { + for _, s := range status.Statuses { + if s == nil { + continue + } + reported = true + switch s.GetState() { + case "failure", "error": + failed = append(failed, s.GetContext()) + case "pending": + pending = true + } + } + } + if checkRuns != nil { + for _, run := range checkRuns.CheckRuns { + if run == nil { + continue + } + reported = true + if run.GetStatus() != "completed" { + pending = true + continue + } + // As ciSummary counts them. + switch run.GetConclusion() { + case "success", "neutral", "skipped", "": + default: + failed = append(failed, run.GetName()) + } + } + } + switch { + case len(failed) > 0: + return "failure", failed + case pending: + return "pending", nil + case reported: + return "success", nil + } + return "", nil +} + +// ciSummaryChange announces a head commit's CI settling: passing, or failing +// where it wasn't before. A check starting, or another failing once one has, +// goes unannounced — a push sets off a delivery per check. +func ciSummaryChange(before, after string, failed []string) string { + if before == after { + return "" + } + switch after { + case "success": + return "Checks passed" + case "failure": + const shown = 3 + names := failed + if len(names) > shown { + names = append(names[:shown:shown], fmt.Sprintf("%d more", len(failed)-shown)) + } + return "Checks failed: " + strings.Join(names, ", ") + } + return "" +} diff --git a/workflows/webhook_changes_test.go b/workflows/webhook_changes_test.go new file mode 100644 index 00000000..a959b0cd --- /dev/null +++ b/workflows/webhook_changes_test.go @@ -0,0 +1,293 @@ +package workflows + +import ( + "slices" + "testing" + + "github.com/google/go-github/v74/github" +) + +// recordPRChanges registers a PR-changed hook that collects what it is told. +func recordPRChanges(t *testing.T) *[]PRChange { + t.Helper() + var got []PRChange + SetPRChangedHook(func(c PRChange) { got = append(got, c) }) + t.Cleanup(func() { SetPRChangedHook(nil) }) + return &got +} + +func user(login string) *github.User { return &github.User{Login: github.Ptr(login)} } + +func TestDeliveriesAnnounceWhatTheyChanged(t *testing.T) { + key := whKey(1) + merged := whPR(1, "head1") + merged.State, merged.Merged = github.Ptr("closed"), github.Ptr(true) + + for _, c := range []struct { + name string + eventType string + event any + wantEvent string + wantChanges []string // each must be among those reported + wantSummary string + wantHead string + }{ + { + name: "an approval", + eventType: "pull_request_review", + event: &github.PullRequestReviewEvent{ + Action: github.Ptr("submitted"), + Review: &github.PullRequestReview{ID: github.Ptr(int64(7)), User: user("rev"), State: github.Ptr("approved")}, + PullRequest: whPR(1, "head1"), + Repo: whRepository(), + Sender: user("rev"), + }, + wantEvent: "pull_request_review.submitted", + wantChanges: []string{"reviews", "metadata"}, + wantSummary: "rev approved", + wantHead: "head1", + }, + { + name: "a review comment", + eventType: "pull_request_review_comment", + event: &github.PullRequestReviewCommentEvent{ + Action: github.Ptr("created"), + Comment: &github.PullRequestComment{ + ID: github.Ptr(int64(10)), Path: github.Ptr("w.go"), Body: github.Ptr("why?"), User: user("rev"), + }, + PullRequest: whPR(1, "head1"), + Repo: whRepository(), + Sender: user("rev"), + }, + wantEvent: "pull_request_review_comment.created", + wantChanges: []string{"comments"}, + wantSummary: "rev commented on w.go", + wantHead: "head1", + }, + { + name: "a conversation comment", + eventType: "issue_comment", + event: &github.IssueCommentEvent{ + Action: github.Ptr("created"), + Issue: &github.Issue{Number: github.Ptr(1), PullRequestLinks: &github.PullRequestLinks{}}, + Comment: &github.IssueComment{ID: github.Ptr(int64(11)), Body: github.Ptr("lgtm"), User: user("rev")}, + Repo: whRepository(), + Sender: user("rev"), + }, + wantEvent: "issue_comment.created", + wantChanges: []string{"comments"}, + wantSummary: "rev commented", + }, + { + name: "a merge", + eventType: "pull_request", + event: &github.PullRequestEvent{Action: github.Ptr("closed"), PullRequest: merged, Repo: whRepository(), Sender: user("lead")}, + wantEvent: "pull_request.closed", + wantChanges: []string{ChangePR, "metadata"}, + wantSummary: "lead merged the PR", + wantHead: "head1", + }, + { + name: "a push", + eventType: "pull_request", + event: &github.PullRequestEvent{Action: github.Ptr("synchronize"), PullRequest: whPR(1, "head2"), Repo: whRepository(), Sender: user("author")}, + wantEvent: "pull_request.synchronize", + wantChanges: []string{ChangePR, ChangeHead}, + wantSummary: "author pushed new commits", + wantHead: "head2", + }, + } { + t.Run(c.name, func(t *testing.T) { + env := newWebhookTestEnv(t) + seedCachedPR(t, env.db, key, "head1") + trackItem(t, env.db, key) + got := recordPRChanges(t) + + applyWebhookEvent(webhookDelivery{eventType: c.eventType, id: "abc", event: c.event}) + + if len(*got) != 1 { + t.Fatalf("announced %d changes, want 1: %+v", len(*got), *got) + } + change := (*got)[0] + if change.Key != key || change.Event != c.wantEvent || change.Summary != c.wantSummary || change.HeadSHA != c.wantHead { + t.Errorf("change = %+v\nwant key %v, event %q, summary %q, head %q", change, key, c.wantEvent, c.wantSummary, c.wantHead) + } + for _, want := range c.wantChanges { + if !slices.Contains(change.Changes, want) { + t.Errorf("changes = %v, want %q among them", change.Changes, want) + } + } + if change.Actor == "" { + t.Error("no actor for a delivery with a sender") + } + }) + } +} + +func TestDeliveriesThatChangeNothingAreNotAnnounced(t *testing.T) { + env := newWebhookTestEnv(t) + got := recordPRChanges(t) + review := func(number int) *github.PullRequestReviewEvent { + return &github.PullRequestReviewEvent{ + Action: github.Ptr("submitted"), + Review: &github.PullRequestReview{ID: github.Ptr(int64(1)), User: user("rev"), State: github.Ptr("approved")}, + PullRequest: whPR(number, "head1"), + Repo: whRepository(), + } + } + + // A PR nothing holds. + applyWebhookEvent(webhookDelivery{eventType: "pull_request_review", event: review(5)}) + // A PR held with no reviews cached to patch. + key := whKey(1) + trackItem(t, env.db, key) + env.db.UpsertPullRequest(key.Number, key.Repo, "head1", "", "") + applyWebhookEvent(webhookDelivery{eventType: "pull_request_review", event: review(1)}) + + if len(*got) != 0 { + t.Errorf("announced %+v for deliveries that changed nothing", *got) + } +} + +func TestCIIsAnnouncedWhenItSettles(t *testing.T) { + env := newWebhookTestEnv(t) + key := whKey(1) + seedCachedPR(t, env.db, key, "head1") + got := recordPRChanges(t) + app := &github.App{ID: github.Ptr(int64(1))} + deliver := func(id int64, name, status, conclusion string) PRChange { + t.Helper() + run := &github.CheckRun{ID: github.Ptr(id), Name: github.Ptr(name), HeadSHA: github.Ptr("head1"), App: app, + Status: github.Ptr(status), PullRequests: []*github.PullRequest{{Number: github.Ptr(1)}}} + if conclusion != "" { + run.Conclusion = github.Ptr(conclusion) + } + *got = nil + applyWebhookEvent(webhookDelivery{eventType: "check_run", event: &github.CheckRunEvent{ + Action: github.Ptr(status), CheckRun: run, Repo: whRepository(), Sender: user("github-actions[bot]"), + }}) + if len(*got) != 1 { + t.Fatalf("announced %d changes for one check run, want 1", len(*got)) + } + return (*got)[0] + } + + steps := []struct { + id int64 + name, status, conclusion string + wantSummary string + }{ + {1, "build", "in_progress", "", ""}, + {2, "lint", "in_progress", "", ""}, + {1, "build", "completed", "failure", "Checks failed: build"}, + // Already failing: another failure isn't news. + {2, "lint", "completed", "failure", ""}, + {3, "build", "in_progress", "", ""}, + {3, "build", "completed", "success", ""}, + {4, "lint", "completed", "success", "Checks passed"}, + } + for _, s := range steps { + change := deliver(s.id, s.name, s.status, s.conclusion) + if change.Summary != s.wantSummary { + t.Errorf("%s %s %s: summary = %q, want %q", s.name, s.status, s.conclusion, change.Summary, s.wantSummary) + } + if !slices.Contains(change.Changes, "ci_status") { + t.Errorf("%s %s: changes = %v, want ci_status", s.name, s.status, change.Changes) + } + if change.Actor != "" { + t.Errorf("CI announced with actor %q", change.Actor) + } + if change.Event != "check_run."+s.status { + t.Errorf("event = %q", change.Event) + } + } +} + +func TestSectionChangesAreAnnounced(t *testing.T) { + wf := reviewRequestedWorkflow() + env := membershipEnv(t, wf) + key := whKey(1) + seedCachedPR(t, env.db, key, "head1") + listIn(t, env.db, wf, 1) + got := recordPRChanges(t) + + applyWebhookEvent(webhookDelivery{eventType: "pull_request_review", event: &github.PullRequestReviewEvent{ + Action: github.Ptr("submitted"), + Review: &github.PullRequestReview{ID: github.Ptr(int64(9)), User: user("me"), State: github.Ptr("approved")}, + PullRequest: requestedOfMe(whPR(1, "head1")), + Repo: whRepository(), + Sender: user("me"), + }}) + if len(*got) != 1 || (*got)[0].Event != "pull_request_review.submitted" { + t.Fatalf("announced %+v before re-evaluating, want just the review", *got) + } + drainReevaluations() + + if len(*got) != 2 { + t.Fatalf("announced %+v, want the review then the section change", *got) + } + sections := (*got)[1] + if sections.Key != key || sections.Event != ChangeSections || !slices.Equal(sections.Changes, []string{ChangeSections}) || sections.Summary != "" { + t.Errorf("section change = %+v", sections) + } +} + +func TestPullRequestSummary(t *testing.T) { + pr := whPR(1, "head1") + for _, c := range []struct { + sender string + event *github.PullRequestEvent + want string + }{ + {"", &github.PullRequestEvent{Action: github.Ptr("closed")}, "Someone closed the PR"}, + {"a", &github.PullRequestEvent{Action: github.Ptr("edited"), Changes: &github.EditChange{Title: &github.EditTitle{From: github.Ptr("old")}}}, "a edited the title"}, + {"a", &github.PullRequestEvent{Action: github.Ptr("edited"), Changes: &github.EditChange{Base: &github.EditBase{Ref: &github.EditRef{From: github.Ptr("develop")}}}}, "a changed the base branch to main"}, + {"a", &github.PullRequestEvent{Action: github.Ptr("review_requested"), RequestedReviewer: user("rev")}, "a requested a review from rev"}, + {"a", &github.PullRequestEvent{Action: github.Ptr("review_requested"), RequestedTeam: &github.Team{Slug: github.Ptr("core")}}, "a requested a review from core"}, + {"a", &github.PullRequestEvent{Action: github.Ptr("labeled"), Label: &github.Label{Name: github.Ptr("bug")}}, "a added the label bug"}, + {"a", &github.PullRequestEvent{Action: github.Ptr("ready_for_review")}, "a marked the PR ready for review"}, + {"a", &github.PullRequestEvent{Action: github.Ptr("auto_merge_enabled")}, ""}, + } { + c.event.PullRequest = pr + if c.sender != "" { + c.event.Sender = user(c.sender) + } + if got := pullRequestSummary(c.event); got != c.want { + t.Errorf("%s: summary = %q, want %q", c.event.GetAction(), got, c.want) + } + } +} + +func TestReviewSummary(t *testing.T) { + review := func(state string) *github.PullRequestReview { + return &github.PullRequestReview{User: user("rev"), State: github.Ptr(state)} + } + for _, c := range []struct { + action string + review *github.PullRequestReview + want string + }{ + {"submitted", review("approved"), "rev approved"}, + {"submitted", review("changes_requested"), "rev requested changes"}, + {"submitted", review("commented"), "rev reviewed"}, + {"dismissed", review("dismissed"), "lead dismissed rev's review"}, + } { + e := &github.PullRequestReviewEvent{Action: github.Ptr(c.action), Review: c.review, Sender: user("lead")} + if got := reviewSummary(e); got != c.want { + t.Errorf("%s %s: summary = %q, want %q", c.action, c.review.GetState(), got, c.want) + } + } +} + +func TestCISummaryChangeNamesAFewFailures(t *testing.T) { + failed := []string{"a", "b", "c", "d", "e"} + if got, want := ciSummaryChange("pending", "failure", failed), "Checks failed: a, b, c, 2 more"; got != want { + t.Errorf("summary = %q, want %q", got, want) + } + if !slices.Equal(failed, []string{"a", "b", "c", "d", "e"}) { + t.Errorf("summarizing rewrote the failures: %v", failed) + } + if got := ciSummaryChange("success", "pending", nil); got != "" { + t.Errorf("a check starting was announced: %q", got) + } +} diff --git a/workflows/webhook_events.go b/workflows/webhook_events.go index fd0f66ed..60c15ad9 100644 --- a/workflows/webhook_events.go +++ b/workflows/webhook_events.go @@ -48,11 +48,17 @@ var fetchReviewThreads = git_tools.GetReviewThreads type heldPR struct { key PRKey pr *github.PullRequest + // changes names what the delivery rewrote for the PR, and summary what it + // says happened (see PRChange). A PR it changed nothing for — its caches + // missing, say — isn't announced. + changes []string + summary string } // applyWebhookEvent applies one delivery to the caches of the PRs it is about, -// then has each of them checked against the workflows again: what changed may -// move it into a section or out of one (see webhook_membership.go). +// tells the clients about each one it changed (see webhook_changes.go), then +// has each of them checked against the workflows again: what changed may move +// it into a section or out of one (see webhook_membership.go). func applyWebhookEvent(d webhookDelivery) { db := config.C().DB if db == nil { @@ -60,6 +66,10 @@ func applyWebhookEvent(d webhookDelivery) { } var held []heldPR reevaluate := true + actor := "" + if e, ok := d.event.(interface{ GetSender() *github.User }); ok { + actor = e.GetSender().GetLogin() + } switch e := d.event.(type) { case *github.PullRequestEvent: held = applyPullRequestEvent(db, d, e) @@ -75,8 +85,11 @@ func applyWebhookEvent(d webhookDelivery) { held = applyIssueCommentEvent(db, d, e) case *github.CheckRunEvent: held = applyCheckRunEvent(db, d, e) + // The app that ran it, which nobody did. + actor = "" case *github.StatusEvent: held = applyStatusEvent(db, d, e) + actor = "" case *github.PushEvent: if applyPushEvent(e) { webhookActivity.applied.Add(1) @@ -90,16 +103,37 @@ func applyWebhookEvent(d webhookDelivery) { return } webhookActivity.applied.Add(1) + event := d.eventName(deliveryAction(d.event)) for _, h := range held { if h.pr != nil { rememberPR(h.key, h.pr) } + notifyPRChanged(PRChange{ + Key: h.key, + Event: event, + Changes: h.changes, + HeadSHA: h.pr.GetHead().GetSHA(), + Actor: actor, + Summary: h.summary, + }) if reevaluate { scheduleReevaluation(h.key) } } } +// deliveryAction is what a delivery says happened: its action, or a commit +// status's state. +func deliveryAction(event any) string { + switch e := event.(type) { + case *github.StatusEvent: + return e.GetState() + case interface{ GetAction() string }: + return e.GetAction() + } + return "" +} + // trackedPR identifies the PR a delivery is about and reports whether we hold // it: a section lists it, or a cycle has cached it. The second matters as much // as the first. A cycle fetches every PR its repositories list, not only the @@ -184,24 +218,28 @@ func applyPullRequestEvent(db *database.DB, d webhookDelivery, e *github.PullReq return nil } detail := d.describe(e.GetAction()) + held := heldPR{key: key, pr: pr, changes: []string{ChangePR}, summary: pullRequestSummary(e)} // A synchronize is a push, and a new base branch changes the diff as surely // as one: both need a fetch. Everything else this event reports — title, // body, draft, state, labels, assignees, review requests — is in the // payload. - if !refetchIfHeadMoved(db, key, pr, detail) { - if e.GetChanges().GetBase() != nil { - refetchPR(key, pr, detail) - } else { - unlock := lockPR(key) - written := refreshFromPRPayload(db, key, pr) - unlock() - logWebhookWrite(key, pr.GetHead().GetSHA(), written, detail) - } + switch { + case refetchIfHeadMoved(db, key, pr, detail): + held.changes = append(held.changes, ChangeHead) + case e.GetChanges().GetBase() != nil: + refetchPR(key, pr, detail) + held.changes = append(held.changes, ChangeHead) + default: + unlock := lockPR(key) + written := refreshFromPRPayload(db, key, pr) + unlock() + logWebhookWrite(key, pr.GetHead().GetSHA(), written, detail) + held.changes = append(held.changes, written...) } recordPayloadMergeability(db, key, pr) updateItemSummaries(db, key, pr) - return []heldPR{{key: key, pr: pr}} + return []heldPR{held} } // refreshFromPRPayload rewrites what the PR object alone determines: who is @@ -292,27 +330,29 @@ func applyReviewEvent(db *database.DB, d webhookDelivery, e *github.PullRequestR if e.GetAction() == "submitted" { pr.RequestedReviewers = withoutReviewer(pr.RequestedReviewers, review.GetUser().GetLogin()) } - held := []heldPR{{key: key, pr: pr}} + held := heldPR{key: key, pr: pr, summary: reviewSummary(e)} detail := d.describe(e.GetAction()) if refetchIfHeadMoved(db, key, pr, detail) { - return held + held.changes = []string{ChangeHead} + return []heldPR{held} } unlock := lockPR(key) defer unlock() reviews, ok := loadCachedReviews(db, key) if !ok { - return held + return []heldPR{held} } if err := cacheReviews(db, key, upsertReview(reviews, review)); err != nil { slog.Error("Failed to cache PR reviews from a webhook", "pr", key.Number, "repo", key.Repo, "error", err) - return held + return []heldPR{held} } // A review clears its author's review request, approves or blocks the PR, // and may settle a required team: everything that reads the reviews. written := append([]string{"reviews"}, refreshFromPRPayload(db, key, pr)...) logWebhookWrite(key, pr.GetHead().GetSHA(), written, detail) - return held + held.changes = written + return []heldPR{held} } // withoutReviewer is reviewers less login's personal review request. @@ -355,11 +395,12 @@ func applyReviewCommentEvent(db *database.DB, d webhookDelivery, e *github.PullR if !ok { return nil } - held := []heldPR{{key: key, pr: pr}} + held := heldPR{key: key, pr: pr, summary: reviewCommentSummary(e)} action := e.GetAction() detail := d.describe(action) if refetchIfHeadMoved(db, key, pr, detail) { - return held + held.changes = []string{ChangeHead} + return []heldPR{held} } unlock := lockPR(key) @@ -382,7 +423,8 @@ func applyReviewCommentEvent(db *database.DB, d webhookDelivery, e *github.PullR written = append(written, "review_threads") } logWebhookWrite(key, pr.GetHead().GetSHA(), written, detail) - return held + held.changes = written + return []heldPR{held} } // patchCachedComments applies a created, edited or deleted comment to the @@ -508,17 +550,18 @@ func applyReviewThreadEvent(db *database.DB, d webhookDelivery, e *github.PullRe if !ok { return nil } - held := []heldPR{{key: key, pr: pr}} + held := heldPR{key: key, pr: pr, summary: reviewThreadSummary(e)} detail := d.describe(action) if refetchIfHeadMoved(db, key, pr, detail) { - return held + held.changes = []string{ChangeHead} + return []heldPR{held} } unlock := lockPR(key) defer unlock() threads, ok := loadCachedReviewThreads(db, key) if !ok { - return held + return []heldPR{held} } resolved := action == "resolved" resolvedBy := "" @@ -563,9 +606,10 @@ func applyReviewThreadEvent(db *database.DB, d webhookDelivery, e *github.PullRe }) } if cacheReviewThreads(db, key, threads) { - logWebhookWrite(key, pr.GetHead().GetSHA(), []string{"review_threads"}, detail) + held.changes = []string{"review_threads"} + logWebhookWrite(key, pr.GetHead().GetSHA(), held.changes, detail) } - return held + return []heldPR{held} } func applyIssueCommentEvent(db *database.DB, d webhookDelivery, e *github.IssueCommentEvent) []heldPR { @@ -583,7 +627,7 @@ func applyIssueCommentEvent(db *database.DB, d webhookDelivery, e *github.IssueC written := patchCachedComments(db, key, e.GetAction(), convertIssueToPRComment(comment)) unlock() logWebhookWrite(key, "", written, d.describe(e.GetAction())) - return []heldPR{{key: key}} + return []heldPR{{key: key, changes: written, summary: issueCommentSummary(e)}} } func applyCheckRunEvent(db *database.DB, d webhookDelivery, e *github.CheckRunEvent) []heldPR { @@ -607,10 +651,10 @@ func applyCheckRunEvent(db *database.DB, d webhookDelivery, e *github.CheckRunEv var held []heldPR for _, number := range uniqueInts(numbers) { if key, ok := trackedPR(db, repo, number); ok { - patchCachedCI(db, key, sha, detail, func(status *github.CombinedStatus, checkRuns *github.ListCheckRunsResults) (*github.CombinedStatus, *github.ListCheckRunsResults) { + written, summary := patchCachedCI(db, key, sha, detail, func(status *github.CombinedStatus, checkRuns *github.ListCheckRunsResults) (*github.CombinedStatus, *github.ListCheckRunsResults) { return status, upsertCheckRun(checkRuns, run) }) - held = append(held, heldPR{key: key}) + held = append(held, heldPR{key: key, changes: written, summary: summary}) } } return held @@ -626,10 +670,10 @@ func applyStatusEvent(db *database.DB, d webhookDelivery, e *github.StatusEvent) var held []heldPR for _, number := range prNumbersForSHA(db, repo, sha) { if key, ok := trackedPR(db, repo, number); ok { - patchCachedCI(db, key, sha, detail, func(status *github.CombinedStatus, checkRuns *github.ListCheckRunsResults) (*github.CombinedStatus, *github.ListCheckRunsResults) { + written, summary := patchCachedCI(db, key, sha, detail, func(status *github.CombinedStatus, checkRuns *github.ListCheckRunsResults) (*github.CombinedStatus, *github.ListCheckRunsResults) { return upsertStatus(status, e), checkRuns }) - held = append(held, heldPR{key: key}) + held = append(held, heldPR{key: key, changes: written, summary: summary}) } } return held @@ -650,26 +694,36 @@ func uniqueInts(values []int) []int { } // patchCachedCI applies patch to the CI cached for the PR's commit sha, and -// when that is the PR's head, to the CI line in its metadata. +// when that is the PR's head, to the CI line in its metadata. It returns the +// fields it wrote, and — when the head's CI just passed or failed — a summary +// saying so. func patchCachedCI(db *database.DB, key PRKey, sha, detail string, - patch func(*github.CombinedStatus, *github.ListCheckRunsResults) (*github.CombinedStatus, *github.ListCheckRunsResults)) { + patch func(*github.CombinedStatus, *github.ListCheckRunsResults) (*github.CombinedStatus, *github.ListCheckRunsResults)) ([]string, string) { unlock := lockPR(key) defer unlock() status, checkRuns, ok := loadCachedCI(db, key, sha) if !ok { - return + return nil, "" } + // Before the patch, which rewrites them in place. + before, _ := ciOutcome(status, checkRuns) status, checkRuns = patch(status, checkRuns) if err := cacheCI(db, key, sha, status, checkRuns); err != nil { slog.Error("Failed to cache CI status from a webhook", "pr", key.Number, "repo", key.Repo, "error", err) - return + return nil, "" } written := []string{"ci_status"} - if _, head, err := db.GetPullRequest(key.Number, key.Repo); err == nil && head == sha && patchMetadataCI(db, key, status, checkRuns) { - written = append(written, "metadata") + summary := "" + if _, head, err := db.GetPullRequest(key.Number, key.Repo); err == nil && head == sha { + if patchMetadataCI(db, key, status, checkRuns) { + written = append(written, "metadata") + } + after, failed := ciOutcome(status, checkRuns) + summary = ciSummaryChange(before, after, failed) } logWebhookWrite(key, sha, written, detail) + return written, summary } // upsertCheckRun puts run into the commit's check runs. The cache holds the diff --git a/workflows/webhook_membership.go b/workflows/webhook_membership.go index e09e5c2f..40015235 100644 --- a/workflows/webhook_membership.go +++ b/workflows/webhook_membership.go @@ -284,8 +284,9 @@ func reevaluateMembershipSafely(db *database.DB, key PRKey, pr *github.PullReque // reevaluateMembership checks pr against every workflow and brings its section // rows in line: added where it now belongs, rewritten — details included, from -// the caches — where it still does, and dropped where it no longer does. The -// caller holds membershipMu. +// the caches — where it still does, and dropped where it no longer does. A +// client is then told the rows changed (ChangeSections), so a review list on +// screen can fetch them again. The caller holds membershipMu. func reevaluateMembership(db *database.DB, key PRKey, pr *github.PullRequest) { if db == nil || !webhookHandles(key) || !cacheServes(db, key, pr) || pr.Number == nil { return @@ -308,7 +309,7 @@ func reevaluateMembership(db *database.DB, key PRKey, pr *github.PullRequest) { defer SetCurrentAuxDataStore(nil) ttl := time.Now().Add(2 * time.Hour).Unix() - dropped := false + dropped, changed := false, false for _, wf := range workflows { member, ok := wf.(webhookMember) if !ok { @@ -338,7 +339,7 @@ func reevaluateMembership(db *database.DB, key PRKey, pr *github.PullRequest) { HeadSHA: pr.GetHead().GetSHA(), }}, "webhook: no longer matches the workflow") webhookActivity.sectionDrops.Add(1) - dropped = true + dropped, changed = true, true slog.Info("Webhook took a PR out of a section", "pr", prName, "section", section.SectionName, "workflow", wf.GetName()) case verdict == memberYes: @@ -349,6 +350,7 @@ func reevaluateMembership(db *database.DB, key PRKey, pr *github.PullRequest) { change.TTL = ttl change.NotifyOnAdd = member.ShouldNotify() applyChange(SerializedFileChange{FileChange: &change, Lines: change.GetLines(2)}, "webhook: matches the workflow") + changed = true if !listed { webhookActivity.sectionAdds.Add(1) slog.Info("Webhook put a PR into a section", "pr", prName, "section", section.SectionName, "workflow", wf.GetName()) @@ -358,4 +360,12 @@ func reevaluateMembership(db *database.DB, key PRKey, pr *github.PullRequest) { if dropped { pruneOrphanedItems() } + if changed { + notifyPRChanged(PRChange{ + Key: key, + Event: ChangeSections, + Changes: []string{ChangeSections}, + HeadSHA: pr.GetHead().GetSHA(), + }) + } } diff --git a/workflows/webhooks.go b/workflows/webhooks.go index 3f9cd2c7..e301a3b8 100644 --- a/workflows/webhooks.go +++ b/workflows/webhooks.go @@ -64,12 +64,17 @@ type webhookDelivery struct { event any } +// eventName is the delivery's event and action, "pull_request.synchronize". +func (d webhookDelivery) eventName(action string) string { + if action == "" { + return d.eventType + } + return d.eventType + "." + action +} + // describe names the delivery for logs and the action log. func (d webhookDelivery) describe(action string) string { - name := d.eventType - if action != "" { - name += "." + action - } + name := d.eventName(action) if d.id == "" { return name } From e020b8bac556c62e092b948628a96b0728c008b0 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 26 Oct 2026 18:20:53 +0000 Subject: [PATCH 2/2] Follow #305's change events in the live refresh - Read again, quietly, when the event stream comes back after dropping (onReconnect, used by usePRUpdates), since a Server.PRUpdated sent while it was down is lost, as #305 does for plugin and AI changes. Unlike useServerChanges it skips the first connect, which only repeats the read the page made on load. - Stop reloading the plugin output after a push: the plugins the push re-runs now announce themselves, and the reload put the panel through a loading state. - Use #305's isChangeFor in place of the identical isUpdateFor. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AMPYZg3x4vXfFzqgvJ1AbU --- CLAUDE.md | 2 +- bun_client/frontend/src/components/PRList.tsx | 8 ++-- bun_client/frontend/src/components/Review.tsx | 18 +++------ bun_client/frontend/src/hooks/usePRUpdates.ts | 37 ++++++++++++------- .../frontend/src/server_events.test.tsx | 28 +++++++++++--- bun_client/frontend/src/server_events.ts | 36 +++++++++++------- docs/web_client_features.md | 9 +++-- 7 files changed, 83 insertions(+), 55 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 60e1e31f..22555705 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -74,7 +74,7 @@ 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, `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 the live refresh: `usePRUpdates` batches `Server.PRUpdated` bursts, the review re-reads the PR in place (`Review.tsx` `refreshLive`, holding a new diff while a comment is being written) and the list its rows, with a toast saying what happened — 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 +- `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, `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 the live refresh: `usePRUpdates` batches `Server.PRUpdated` bursts (and reads again when the stream reconnects after dropping, `onReconnect`), the review re-reads the PR in place (`Review.tsx` `refreshLive`, holding a new diff while a comment is being written) and the list its rows, with a toast saying what happened — 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`). diff --git a/bun_client/frontend/src/components/PRList.tsx b/bun_client/frontend/src/components/PRList.tsx index 1938ca11..a9aa87c7 100644 --- a/bun_client/frontend/src/components/PRList.tsx +++ b/bun_client/frontend/src/components/PRList.tsx @@ -35,7 +35,7 @@ import { } from '../pr_list_utils'; import type { Facet, PRState, ReviewItem } from '../pr_list_utils'; import { teamChips } from '../team_utils'; -import { CHANGE_SECTIONS, isUpdateFor, prUpdateMessage, type PRUpdate } from '../server_events'; +import { CHANGE_SECTIONS, isChangeFor, prUpdateMessage, type PRUpdate } from '../server_events'; interface PRListProps { onOpenReview: (owner: string, repo: string, number: number) => void; @@ -314,9 +314,7 @@ export default function PRList({ if (seq !== refreshSeq.current) return; // A PR it showed, or shows now: one that just left a section was in it. const shown = updates.filter(update => - [...items, ...loaded].some(item => - isUpdateFor(update, item.owner, item.repo, item.number) - ) + [...items, ...loaded].some(item => isChangeFor(update, item)) ); setItems(loaded); setLoadError(false); @@ -334,7 +332,7 @@ export default function PRList({ usePRUpdates( update => update.changes.includes(CHANGE_SECTIONS) || - items.some(item => isUpdateFor(update, item.owner, item.repo, item.number)), + items.some(item => isChangeFor(update, item)), refreshLive ); diff --git a/bun_client/frontend/src/components/Review.tsx b/bun_client/frontend/src/components/Review.tsx index db95007d..0631c7a3 100644 --- a/bun_client/frontend/src/components/Review.tsx +++ b/bun_client/frontend/src/components/Review.tsx @@ -28,13 +28,7 @@ import { useLsp } from '../hooks/useLsp'; import { useIsMobile } from '../hooks/useMediaQuery'; import { usePluginOutputs } from '../hooks/usePluginOutputs'; import { usePRUpdates } from '../hooks/usePRUpdates'; -import { - CHANGE_HEAD, - changesPR, - isUpdateFor, - prUpdateMessage, - type PRUpdate, -} from '../server_events'; +import { changesPR, isChangeFor, prUpdateMessage, type PRUpdate } from '../server_events'; import { annotationCommentBody, collectPRAnnotations, indexAnnotations } from '../annotation_utils'; import { parseDiff } from '../diff_utils'; import { buildPendingPreviews, reviewSubmittedMessage } from '../review_preview_utils'; @@ -587,18 +581,16 @@ export default function Review({ setOutdatedComments(unlessSame(res.outdated_comments || [])); setReviews(unlessSame(res.reviews || [])); setMetadata(unlessSame(res.metadata || null)); - // Comments can change what the AI features cover, and a push re-runs - // the plugins. + // Comments and a push can change what the AI features' results cover, + // which no AI change event reports. The plugins a push re-runs + // announce themselves (usePluginOutputs). loadAIOutputs(); - if (diffChanged || updates.some(u => u.changes.includes(CHANGE_HEAD))) { - loadPluginOutputs(); - } const message = held ? 'Refreshed with the latest changes' : prUpdateMessage(updates); if (message) showToast(message, 'info'); }; usePRUpdates( - update => isUpdateFor(update, owner, repo, number) && changesPR(update), + update => isChangeFor(update, { owner, repo, number }) && changesPR(update), updates => refreshLive(updates) ); diff --git a/bun_client/frontend/src/hooks/usePRUpdates.ts b/bun_client/frontend/src/hooks/usePRUpdates.ts index c734911d..f522ad8a 100644 --- a/bun_client/frontend/src/hooks/usePRUpdates.ts +++ b/bun_client/frontend/src/hooks/usePRUpdates.ts @@ -1,11 +1,19 @@ import { useEffect, useRef } from 'react'; -import { onPRUpdates, serverEvents, type PRUpdate, type ServerEvents } from '../server_events'; +import { + onPRUpdates, + onReconnect, + serverEvents, + type PRUpdate, + type ServerEvents, +} from '../server_events'; /** * Calls `onUpdates` with each batch of the server's PR updates (a webhook - * delivery changing a PR) that `accept` lets through. Both are read fresh on - * every update, so they can close over the latest state without the stream - * being re-subscribed each render. + * delivery changing a PR) that `accept` lets through, and with none when the + * event stream comes back after dropping: whatever was sent meanwhile is lost, + * so the page reads again, quietly. Both are read fresh on every update, so + * they can close over the latest state without the stream being re-subscribed + * each render. */ export function usePRUpdates( accept: (update: PRUpdate) => boolean, @@ -17,13 +25,16 @@ export function usePRUpdates( acceptRef.current = accept; onUpdatesRef.current = onUpdates; - useEffect( - () => - onPRUpdates( - events, - update => acceptRef.current(update), - updates => onUpdatesRef.current(updates) - ), - [events] - ); + useEffect(() => { + const offUpdates = onPRUpdates( + events, + update => acceptRef.current(update), + updates => onUpdatesRef.current(updates) + ); + const offReconnect = onReconnect(events, () => onUpdatesRef.current([])); + return () => { + offUpdates(); + offReconnect(); + }; + }, [events]); } diff --git a/bun_client/frontend/src/server_events.test.tsx b/bun_client/frontend/src/server_events.test.tsx index 4d27f804..afc7cb76 100644 --- a/bun_client/frontend/src/server_events.test.tsx +++ b/bun_client/frontend/src/server_events.test.tsx @@ -8,8 +8,8 @@ import { heartbeatReducer, INITIAL_HEARTBEAT_STATE, isChangeFor, - isUpdateFor, onPRUpdates, + onReconnect, parseAIOutputChanged, parseHeartbeat, parsePluginOutputChanged, @@ -319,11 +319,27 @@ describe('parsePRUpdate', () => { }); }); -describe('isUpdateFor', () => { - test('matches the PR however it is capitalized', () => { - expect(isUpdateFor(UPDATE, 'c-hipple', 'Code-Review-Server', 303)).toBe(true); - expect(isUpdateFor(UPDATE, 'C-Hipple', 'code-review-server', 302)).toBe(false); - expect(isUpdateFor(UPDATE, 'C-Hipple', 'gtdbot', 303)).toBe(false); +describe('onReconnect', () => { + test('calls back when the stream comes back after dropping, not on its first connect', () => { + const { events, opened } = hub(); + const offEvents = events.on(PR_UPDATED_EVENT, () => {}); + let reconnects = 0; + const off = onReconnect(events, () => reconnects++); + + opened[0].connect(); + expect(reconnects).toBe(0); + opened[0].fail(); + opened[0].connect(); + expect(reconnects).toBe(1); + // An open with no drop before it brings nothing new. + opened[0].connect(); + expect(reconnects).toBe(1); + + off(); + opened[0].fail(); + opened[0].connect(); + expect(reconnects).toBe(1); + offEvents(); }); }); diff --git a/bun_client/frontend/src/server_events.ts b/bun_client/frontend/src/server_events.ts index e3af953e..c7d08fa1 100644 --- a/bun_client/frontend/src/server_events.ts +++ b/bun_client/frontend/src/server_events.ts @@ -157,8 +157,6 @@ export function heartbeatReducer(state: HeartbeatState, action: HeartbeatAction) */ export const PR_UPDATED_EVENT = 'Server.PRUpdated'; -/** `changes` when the PR's head or base moved: its diff may be new. */ -export const CHANGE_HEAD = 'head'; /** `changes` when only the review list's rows for the PR changed. */ export const CHANGE_SECTIONS = 'sections'; @@ -207,18 +205,6 @@ export function parsePRUpdate(data: unknown): PRUpdate | null { }; } -/** - * Whether `update` is about the PR owner/repo#number. GitHub and the config - * can spell an owner or repository in different cases. - */ -export function isUpdateFor(update: PRUpdate, owner: string, repo: string, number: number) { - return ( - update.number === number && - update.owner.toLowerCase() === owner.toLowerCase() && - update.repo.toLowerCase() === repo.toLowerCase() - ); -} - /** Whether `update` changed the PR itself, not just the review list's rows for it. */ export function changesPR(update: PRUpdate): boolean { return update.changes.some(change => change !== CHANGE_SECTIONS); @@ -297,6 +283,28 @@ export function onPRUpdates( }; } +/** + * Calls `listener` each time the stream comes back after dropping. Anything + * sent while it was down never arrives, so that is when to read again. Unlike + * `onOpen` it skips the stream's first connect, which brings nothing a page + * that has just loaded is missing. Returns the unsubscribe. + */ +export function onReconnect(events: ServerEvents, listener: () => void): () => void { + let dropped = false; + const offError = events.onError(() => { + dropped = true; + }); + const offOpen = events.onOpen(() => { + if (!dropped) return; + dropped = false; + listener(); + }); + return () => { + offError(); + offOpen(); + }; +} + type MessageListener = (event: MessageEvent) => void; /** The parts of EventSource ServerEvents uses, so tests can stand in for it. */ diff --git a/docs/web_client_features.md b/docs/web_client_features.md index 131c1395..8d0356c6 100644 --- a/docs/web_client_features.md +++ b/docs/web_client_features.md @@ -680,7 +680,9 @@ With [`[Webhooks]`](webhooks.md) configured, the server sends a and the bridge relays it over `/api/events` like the heartbeat. `onPRUpdates` collects a burst — a push sets off a delivery per check, then the section change — into one batch: the first update opens it and the rest join it for 400 ms, so the -page refreshes once per burst and never waits longer than that. +page refreshes once per burst and never waits longer than that. When the event +stream comes back after dropping (`onReconnect`), both views read again quietly, +since whatever was sent meanwhile is lost. - **Review** (`Review.tsx`, `refreshLive`): an update for the open PR, other than one that only touched the list's rows (`sections`), re-calls `GetPR` — served from the @@ -693,8 +695,9 @@ page refreshes once per burst and never waits longer than that. comment is done, and closing the form brings it in. With a filtered diff on screen the new full diff is kept for comment positions, and the filtered one is asked for again after a push or the user's own review or comment (either can - move it), and swapped in only if the answer changed. A push also reloads the - plugin outputs; every refresh reloads the AI outputs' counts. + move it), and swapped in only if the answer changed. Every refresh reloads the + AI outputs' counts, which a push or a comment can make stale; the plugins a + push re-runs announce themselves (`Server.PluginOutputChanged`). - **List** (`PRList.tsx`): any update re-calls `GetAllReviews` in place — filters, collapsed sections and scroll kept, no spinner, no plugin warm-up — and the rows of the PRs it named flash with the accent colour for a moment (`.crs-row-updated`,