diff --git a/DESIGN.md b/DESIGN.md index c0d30965..60bd789f 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -310,7 +310,10 @@ such detail into a token. New layout spacing must use the scale. identity ties. Recent first is one flat activity-ordered list labeling each row with its repository. Rows retain compact tree density, truncation, ages and metadata. - Show only current heads with changes. The current stream uses `--accent-bg`, + Show only current heads with changes; closed and merged GitHub pull requests + are not current. A pull request row shows a pull request (or draft) icon + labeled for assistive technology, `#N title`, and “GitHub · @author” first in + its metadata. The current stream uses `--accent-bg`, accent text and `aria-current="page"`. Unopened arrivals show a small accent dot labeled “New changes”; collapsed repositories carry their streams' dots. Repository collapse state persists per browser. @@ -323,6 +326,13 @@ such detail into a token. New layout spacing must use the scale. `aria-current="true"` with accent text on `--selected` for the open review's repository. A mark (“Open latest in {repo}”) opens that repository's most active stream; the rail stays collapsed. +- A GitHub pull request review shows a banner on `--panel` above the review + notices: a state icon, muted `#N` and the semibold title, a status `Badge` + (Open success, Draft neutral, Merged accent, Closed error), `@author`, muted + monospace head → base, an outline-muted “Open on GitHub” `LinkButton`, and a + “Show description” disclosure, collapsed by default and persisted per + browser. The description is `MarkdownText`, bounded by `--reading-width`, and + scrolls within the banner rather than pushing the diff away. - A newer head for the viewed stream shows a one-line `--accent-bg` notice below the header with an outline-muted “View latest” action. Never swap the open review. - Unopened arrivals are marked only by the column's and rail's accent dots and @@ -433,6 +443,8 @@ with a semantic variant instead of overriding it. | Need | Component (`apps/web/src/components`) | | -------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------- | +| Navigation styled as a button | `link-button` (keeps link semantics; `external` opens a new tab) | +| Untrusted Markdown prose | `markdown` (`MarkdownText`: no raw HTML, images as links) | | Action, link-styled trigger | `ui/button` (variants: default, outline, outline-muted, outline-check, ghost, shell, rail, subtle, destructive, destructive-solid, link) | | Icon-only action | `icon-button` (label is name and tooltip) | | Primary action plus menu | `split-button` | diff --git a/apps/web/package.json b/apps/web/package.json index da630f6b..1c0a94de 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -22,6 +22,8 @@ "lucide-react": "catalog:", "react": "catalog:", "react-dom": "catalog:", + "react-markdown": "catalog:", + "remark-gfm": "catalog:", "shadcn": "catalog:", "tw-animate-css": "catalog:" }, diff --git a/apps/web/src/App.tsx b/apps/web/src/App.tsx index 82f74c85..8aaea01d 100644 --- a/apps/web/src/App.tsx +++ b/apps/web/src/App.tsx @@ -6,6 +6,7 @@ import { FileExplorerNav } from "./FileExplorerNav.tsx"; import { Header } from "./Header.tsx"; import { Navigation } from "./Navigation.tsx"; import { NewerSnapshotNotice } from "./ProjectsNav.tsx"; +import { PullRequestBanner } from "./PullRequestBanner.tsx"; import { CopyDialog } from "./review.tsx"; import { capabilityEnabled, useSession } from "./session-context.tsx"; import { useSidebarState } from "./sidebar-context.tsx"; @@ -22,6 +23,7 @@ export function App() {
+ diff --git a/apps/web/src/ContextIdentity.tsx b/apps/web/src/ContextIdentity.tsx index edd5e09e..a61b5d44 100644 --- a/apps/web/src/ContextIdentity.tsx +++ b/apps/web/src/ContextIdentity.tsx @@ -32,7 +32,7 @@ import { type ProjectGroup, type ProjectStream, } from "./project-nav.ts"; -import { NewMark, useNow } from "./ProjectsNav.tsx"; +import { NewMark, StreamIcon, useNow } from "./ProjectsNav.tsx"; import { useCatalog } from "./session-context.tsx"; import { useShellPolicy } from "./shell-policy.ts"; @@ -204,7 +204,7 @@ function BranchItem({ return ( -
); } diff --git a/apps/web/src/project-nav.ts b/apps/web/src/project-nav.ts index b9d81309..d0be195f 100644 --- a/apps/web/src/project-nav.ts +++ b/apps/web/src/project-nav.ts @@ -30,10 +30,22 @@ export function projectKey(context: ApiContext) { return context.repositoryId ?? `name:${context.name}`; } +/** The GitHub pull request a stream reviews; the server alone authors it. */ +export function pullRequestOf(context: ApiContext) { + return context.observation?.pullRequest; +} + +/** Closed and merged pull requests leave current work; their reviews remain. */ +function currentWork(context: ApiContext) { + const state = pullRequestOf(context)?.state; + return state !== "closed" && state !== "merged"; +} + export function projectGroups(heads: ApiContext[], seen: SeenStreams | null) { const groups = new Map(); for (const context of heads) { - if (context.stale || !contextHasChanges(context)) continue; + if (context.stale || !contextHasChanges(context) || !currentWork(context)) + continue; const id = projectKey(context); const group = groups.get(id) ?? { id, @@ -126,6 +138,8 @@ export function streamMatches(stream: ProjectStream, query: string) { const { context } = stream; const text = [ streamTitle(context), + context.branch ?? "", + pullRequestOf(context)?.author.login ?? "", streamWorktree(context), streamHost(context), ...streamSessions(context), @@ -147,6 +161,8 @@ export function repoMonogram(name: string) { } export function streamTitle(context: ApiContext) { + const pull = pullRequestOf(context); + if (pull) return `#${pull.number} ${pull.title}`; return context.branch ?? context.observation?.branch ?? "Detached HEAD"; } @@ -162,10 +178,14 @@ export function streamMeta( )[0]?.harness ?? context.observation?.agentSession?.harness ?? ""; + const pull = pullRequestOf(context); return [ + pull ? "GitHub" : "", + pull?.draft ? "Draft" : "", + pull ? `@${pull.author.login}` : "", worktree, harness, - multipleHosts ? streamHost(context) : "", + multipleHosts && !pull ? streamHost(context) : "", relativeAge(contextActivity(context), now), ] .filter(Boolean) diff --git a/apps/web/src/review.css b/apps/web/src/review.css index e7b9fae0..7f4ecbf9 100644 --- a/apps/web/src/review.css +++ b/apps/web/src/review.css @@ -145,6 +145,11 @@ padding: var(--space-2) var(--page-gutter); border-bottom: 1px solid var(--border-muted); } +/* A long description scrolls within the banner and never pushes the diff away. */ +.pull-request-description { + max-height: min(40dvh, calc(6 * var(--space-16))); + overflow-y: auto; +} .comments-panel { flex: 1 0 calc(4 * var(--space-12)); overflow: auto; diff --git a/apps/web/test/browser/identity.spec.ts b/apps/web/test/browser/identity.spec.ts index 6eca8a25..579eb231 100644 --- a/apps/web/test/browser/identity.spec.ts +++ b/apps/web/test/browser/identity.spec.ts @@ -326,3 +326,170 @@ test("logout failure stays recoverable and success disposes account UI", async ( await page.getByRole("button", { name: "Sign out", exact: true }).click(); await expect(page.getByRole("heading", { name: "Sign in" })).toBeVisible(); }); + +test("GitHub settings stay absent until the server configures the integration", async ({ + page, +}) => { + await remote(page); + await page.route("**/api/v2/github/connection", (route) => + route.fulfill({ + status: 404, + json: { detail: "GitHub integration is not configured" }, + }), + ); + await page.goto("/settings"); + await expect(page.getByRole("heading", { name: "API tokens" })).toBeVisible(); + await expect(page.getByRole("heading", { name: "GitHub" })).toHaveCount(0); +}); + +test("connecting GitHub starts authorization with CSRF and reports the callback once", async ({ + page, +}) => { + await remote(page); + await page.route("**/api/v2/github/connection", (route) => { + if (route.request().method() === "POST") { + expect(route.request().headers()["x-csrf-token"]).toBe("session-csrf"); + return route.fulfill({ + json: { authorizeUrl: "https://github.test/login/oauth/authorize" }, + }); + } + return route.fulfill({ + json: { + connected: false, + installUrl: "https://github.com/apps/diffx/installations/new", + }, + }); + }); + await page.route("https://github.test/**", (route) => + route.fulfill({ contentType: "text/html", body: "

Authorize

" }), + ); + await page.goto("/settings?github=cancelled"); + await expect( + page.getByText("GitHub authorization was cancelled."), + ).toBeVisible(); + expect(new URL(page.url()).search).toBe(""); + await expect( + page.getByRole("link", { name: "Install on repositories" }), + ).toHaveAttribute("href", "https://github.com/apps/diffx/installations/new"); + await page + .getByRole("button", { name: "Connect GitHub", exact: true }) + .click(); + await expect(page.getByRole("heading", { name: "Authorize" })).toBeVisible(); + expect(page.url()).toBe("https://github.test/login/oauth/authorize"); +}); + +test("followed repositories toggle with CSRF, resync, and lost access stays listed", async ({ + page, +}) => { + await remote(page); + await page.route("**/api/v2/github/connection", (route) => + route.fulfill({ + json: { + connected: true, + login: "alice-gh", + state: "active", + installUrl: "https://github.com/apps/diffx/installations/new", + }, + }), + ); + await page.route("**/api/v2/github/repositories", (route) => + route.fulfill({ + json: { + repositories: [ + { + id: 10, + fullName: "octo/app", + private: true, + followed: false, + lost: false, + }, + { + id: 11, + fullName: "octo/gone", + private: false, + followed: true, + lost: true, + }, + ], + }, + }), + ); + const follows: string[] = []; + await page.route("**/api/v2/github/follows/*", (route) => { + expect(route.request().headers()["x-csrf-token"]).toBe("session-csrf"); + follows.push( + `${route.request().method()} ${new URL(route.request().url()).pathname}`, + ); + return route.fulfill({ status: 204 }); + }); + let resynced = false; + await page.route("**/api/v2/github/resync", (route) => { + expect(route.request().headers()["x-csrf-token"]).toBe("session-csrf"); + resynced = true; + return route.fulfill({ status: 204 }); + }); + await page.goto("/settings"); + await expect(page.getByText("Connected as @alice-gh.")).toBeVisible(); + const app = page.getByRole("checkbox", { name: "octo/app" }); + await expect(app).not.toBeChecked(); + await expect(page.getByRole("img", { name: "Private" })).toBeVisible(); + await expect(page.getByText("Access lost")).toBeVisible(); + await app.click(); + await expect(app).toBeChecked(); + await expect(page.getByText("Following octo/app.")).toBeVisible(); + await page.getByRole("checkbox", { name: "octo/gone" }).click(); + await expect(page.getByRole("checkbox", { name: "octo/gone" })).toHaveCount( + 0, + ); + expect(follows).toEqual([ + "PUT /api/v2/github/follows/10", + "DELETE /api/v2/github/follows/11", + ]); + await page.getByRole("button", { name: "Resync pull requests" }).click(); + await expect( + page.getByText("Refreshing pull requests of followed repositories."), + ).toBeVisible(); + expect(resynced).toBe(true); +}); + +test("expired GitHub authorization asks to connect again and disconnect confirms", async ({ + page, +}) => { + await remote(page); + let connected = true; + await page.route("**/api/v2/github/connection", (route) => { + if (route.request().method() === "DELETE") { + expect(route.request().headers()["x-csrf-token"]).toBe("session-csrf"); + connected = false; + return route.fulfill({ status: 204 }); + } + return route.fulfill({ + json: connected + ? { + connected: true, + login: "alice-gh", + state: "expired", + installUrl: "https://github.com/apps/diffx/installations/new", + } + : { + connected: false, + installUrl: "https://github.com/apps/diffx/installations/new", + }, + }); + }); + await page.goto("/settings"); + await expect(page.getByText(/expired\. Connect again/)).toBeVisible(); + await expect( + page.getByRole("button", { name: "Connect GitHub again" }), + ).toBeVisible(); + await expect(page.getByRole("heading", { name: "Repositories" })).toHaveCount( + 0, + ); + await page.getByRole("button", { name: "Disconnect", exact: true }).click(); + const confirm = page.getByRole("dialog", { name: "Disconnect GitHub?" }); + await confirm.getByRole("button", { name: "Disconnect GitHub" }).click(); + await expect(confirm).toBeHidden(); + await expect( + page.getByRole("button", { name: "Connect GitHub", exact: true }), + ).toBeVisible(); +}); diff --git a/apps/web/test/browser/local-workspace.spec.ts b/apps/web/test/browser/local-workspace.spec.ts index 71233fd0..8eaf194a 100644 --- a/apps/web/test/browser/local-workspace.spec.ts +++ b/apps/web/test/browser/local-workspace.spec.ts @@ -1,6 +1,6 @@ import { type Page, type Route } from "@playwright/test"; import { expect, fileToggle, test, withReviewNavigation } from "./fixtures.ts"; -import type { ApiContext, ApiRepositoryDiff } from "@diffx/api"; +import type { ApiContext, ApiPullRequest, ApiRepositoryDiff } from "@diffx/api"; import { comparisonKind, isDiffMode, @@ -2935,3 +2935,117 @@ test("saved comparison preferences belong to the active account", async ({ await page.goto("/contexts/local-review"); await ready(page, "committed-v1"); }); + +test("pull request reviews show GitHub identity and a sanitized description; closed ones leave the column", async ({ + page, +}) => { + const state = await localWorkspace(page); + await page.route("**/api/v2/bootstrap", (route) => + route.fulfill({ + json: { + mode: "remote", + signupEnabled: false, + user: { id: "account-a", username: "alice" }, + csrfToken: "csrf", + collect: false, + }, + }), + ); + const pull = { + provider: "github", + repositoryId: 10, + fullName: "octo/app", + number: 12, + url: "https://github.com/octo/app/pull/12", + title: "Add search", + body: [ + "Adds **search**.", + "", + "", + '', + "", + "[unsafe](javascript:window.injected=true) and ![logo](https://example.com/logo.png)", + ].join("\n"), + state: "open", + draft: false, + author: { login: "mona", url: "https://github.com/mona" }, + baseRef: "main", + headRef: "search", + headRepository: "mona/app", + createdAt: 1, + updatedAt: 2, + } satisfies ApiPullRequest; + state.context.streamId = "pr-stream"; + state.context.latestContextId = state.context.id; + state.context.branch = "search"; + state.context.observation = { + sourceId: "github", + hostname: "github.com", + runId: "", + agent: "", + trigger: "github", + repositoryKey: "repo", + repositoryName: "app", + remoteUrl: "https://github.com/octo/app.git", + checkoutKey: "pr-12", + root: pull.url, + worktreeName: "PR #12", + branch: "search", + head: "a".repeat(40), + collectedAt: 2, + collectorVersion: "test", + pullRequest: pull, + }; + const closed: ApiContext = { + ...state.context, + id: "closed-head", + streamId: "closed-stream", + latestContextId: "closed-head", + observation: { + ...state.context.observation, + pullRequest: { ...pull, number: 9, title: "Old work", state: "merged" }, + }, + }; + await page.route("**/api/v2/contexts?*", (route) => + route.fulfill({ + json: { contexts: [state.context, closed], nextCursor: null }, + }), + ); + await page.goto("/contexts/local-review?scope=all"); + await ready(page); + const nav = page.getByRole("navigation", { name: "Reviews", exact: true }); + const row = nav.getByRole("link", { name: /#12 Add search/ }); + await expect(row).toContainText("GitHub · @mona"); + await expect(row.getByRole("img", { name: "Pull request" })).toBeVisible(); + await expect(nav.getByText(/Old work/)).toHaveCount(0); + + const banner = page.getByRole("region", { name: "Pull request #12" }); + await expect( + banner.getByRole("heading", { name: "#12 Add search" }), + ).toBeVisible(); + await expect(banner.getByText("Open", { exact: true })).toBeVisible(); + await expect(banner).toContainText("@mona"); + await expect(banner).toContainText("Merging search"); + await expect(banner).toContainText("into main"); + await expect( + banner.getByRole("link", { name: "Open on GitHub" }), + ).toHaveAttribute("href", pull.url); + await expect(banner.getByText("Adds")).toBeHidden(); + await banner.getByRole("button", { name: "Show description" }).click(); + await expect(banner.getByText("search", { exact: true })).toHaveJSProperty( + "tagName", + "STRONG", + ); + await expect(banner.locator("script, img")).toHaveCount(0); + await expect(banner).not.toContainText(" "injected" in window)).toBe(false); + expect( + await banner.getByRole("link", { name: "unsafe" }).getAttribute("href"), + ).not.toMatch(/^javascript:/i); + await expect( + banner.getByRole("link", { name: "Image: logo" }), + ).toHaveAttribute("href", "https://example.com/logo.png"); + await banner.getByRole("button", { name: "Hide description" }).click(); + await expect(banner.getByText("Adds")).toBeHidden(); +}); diff --git a/apps/web/test/gallery/gallery.spec.ts b/apps/web/test/gallery/gallery.spec.ts index 9701d4a2..dfc6a745 100644 --- a/apps/web/test/gallery/gallery.spec.ts +++ b/apps/web/test/gallery/gallery.spec.ts @@ -14,6 +14,7 @@ const sections = [ "rows", "choices", "states", + "markdown", ] as const; async function open(page: import("@playwright/test").Page, query: string) { diff --git a/apps/web/test/gallery/goldens/button-dark-linux.png b/apps/web/test/gallery/goldens/button-dark-linux.png index 75da12f2..e9582711 100644 Binary files a/apps/web/test/gallery/goldens/button-dark-linux.png and b/apps/web/test/gallery/goldens/button-dark-linux.png differ diff --git a/apps/web/test/gallery/goldens/button-light-linux.png b/apps/web/test/gallery/goldens/button-light-linux.png index 799929e1..bb295412 100644 Binary files a/apps/web/test/gallery/goldens/button-light-linux.png and b/apps/web/test/gallery/goldens/button-light-linux.png differ diff --git a/apps/web/test/gallery/goldens/button-touch-linux.png b/apps/web/test/gallery/goldens/button-touch-linux.png index 6e9e7536..f028ab4a 100644 Binary files a/apps/web/test/gallery/goldens/button-touch-linux.png and b/apps/web/test/gallery/goldens/button-touch-linux.png differ diff --git a/apps/web/test/gallery/goldens/markdown-dark-linux.png b/apps/web/test/gallery/goldens/markdown-dark-linux.png new file mode 100644 index 00000000..c7358b3f Binary files /dev/null and b/apps/web/test/gallery/goldens/markdown-dark-linux.png differ diff --git a/apps/web/test/gallery/goldens/markdown-light-linux.png b/apps/web/test/gallery/goldens/markdown-light-linux.png new file mode 100644 index 00000000..807c51fa Binary files /dev/null and b/apps/web/test/gallery/goldens/markdown-light-linux.png differ diff --git a/apps/web/test/project-nav.test.ts b/apps/web/test/project-nav.test.ts index 44b62729..362fae05 100644 --- a/apps/web/test/project-nav.test.ts +++ b/apps/web/test/project-nav.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import { test } from "node:test"; -import type { ApiContext } from "@diffx/api"; +import type { ApiContext, ApiPullRequest } from "@diffx/api"; import { baselineSeen, latestStream, @@ -12,6 +12,7 @@ import { repoMonogram, streamMatches, streamMeta, + streamTitle, } from "../src/project-nav.ts"; import { compareContextActivity, @@ -399,3 +400,77 @@ test("branch filter matches every word across branch, worktree, host and session assert.deepEqual(ids("fix claude"), ["auth"]); assert.deepEqual(ids("auth ci"), []); }); + +function pullRequestHead( + id: string, + pull: Partial = {}, +): ApiContext { + const pullRequest: ApiPullRequest = { + provider: "github", + repositoryId: 10, + fullName: "octo/app", + number: 12, + url: "https://github.com/octo/app/pull/12", + title: "Add search", + body: "", + state: "open", + draft: false, + author: { login: "mona", url: "https://github.com/mona" }, + baseRef: "main", + headRef: "search", + headRepository: "mona/app", + createdAt: 1, + updatedAt: 2, + ...pull, + }; + return head(id, "app", 10, { + branch: "search", + observation: { + sourceId: "github", + hostname: "github.com", + runId: "", + agent: "", + trigger: "github", + repositoryKey: "app", + repositoryName: "app", + remoteUrl: "https://github.com/octo/app.git", + checkoutKey: id, + root: pullRequest.url, + worktreeName: `PR #${pullRequest.number}`, + branch: "search", + head: "a".repeat(40), + collectedAt: 10, + collectorVersion: "test", + pullRequest, + }, + }); +} + +test("closed and merged pull requests leave current work beside open ones", () => { + const groups = projectGroups( + [ + pullRequestHead("open"), + pullRequestHead("draft", { number: 13, draft: true }), + pullRequestHead("closed", { number: 14, state: "closed" }), + pullRequestHead("merged", { number: 15, state: "merged" }), + head("local", "app", 5), + ], + null, + ); + assert.deepEqual( + groups.flatMap((group) => group.streams.map((stream) => stream.context.id)), + ["draft", "open", "local"], + ); +}); + +test("pull request streams show number, title, GitHub and author", () => { + const open = pullRequestHead("open"); + const draft = pullRequestHead("draft", { draft: true }); + assert.equal(streamTitle(open), "#12 Add search"); + assert.equal(streamTitle(head("local", "app", 5)), "branch-local"); + assert.equal(streamMeta(open, true, 10), "GitHub · @mona · now"); + assert.equal(streamMeta(draft, false, 10), "GitHub · Draft · @mona · now"); + const stream = { id: "open", context: open, unseen: false }; + assert.ok(streamMatches(stream, "search mona")); + assert.ok(streamMatches(stream, "#12")); +}); diff --git a/docs-website/src/pages/github-pull-requests.md b/docs-website/src/pages/github-pull-requests.md new file mode 100644 index 00000000..345cdee1 --- /dev/null +++ b/docs-website/src/pages/github-pull-requests.md @@ -0,0 +1,132 @@ +--- +title: GitHub pull requests +description: Review open GitHub pull requests on a diffx server. +--- + +# GitHub pull requests + +A `diffx-server` can review the open pull requests of GitHub repositories you +follow. Each pull request is its own review, grouped with your other reviews of +that repository. Its row in the Reviews column shows its number, title, GitHub +and author. A banner above the diff shows its state, branches, a link to GitHub +and its description. Closed and merged pull requests leave the Reviews column; +their reviews stay reachable through search until they expire. + +This needs the distributed server, a GitHub App you create, and the +`diffx-fetcher` container. Local `diffx` has no GitHub integration. + +## How it works + +1. GitHub sends pull request events to the server's webhook. +2. The server records which pull requests need a refresh. Repeated events + coalesce; redelivered events have no effect. +3. `diffx-fetcher` leases one pull request at a time over a private port. The + server gives it a short-lived token that can only read that repository's + contents. +4. The fetcher keeps a partial clone of each repository, downloads only the + files the pull request changes, and computes the diff offline. +5. The server publishes the diff to every account that follows the repository + and can still read it on GitHub. + +The server checks access again when you open Settings, follow a repository or +resync, and before delivering to an account whose last check is older than +eight hours. GitHub does not retry failed webhook deliveries, so the server +also reconciles at startup and when you choose **Resync pull requests**. + +## Create the GitHub App + +In GitHub, open **Settings → Developer settings → GitHub Apps → New GitHub +App** (or the same page of an organization). + +- **Homepage URL**: your diffx server, such as `https://diffx.example.com`. +- **Callback URL**: `https://diffx.example.com/api/v2/github/callback`. +- **Expire user authorization tokens**: on. +- **Request user authorization (OAuth) during installation**: off. +- **Webhook URL**: `https://diffx.example.com/api/v2/github/webhooks`. +- **Webhook secret**: a long random value, such as the output of + `openssl rand -hex 32`. +- **Repository permissions**: Contents read-only, Pull requests read-only, + Metadata read-only. +- **Subscribe to events**: Pull request. + +Installation and authorization events are delivered without a subscription. +After creating the app, note its App ID and Client ID, generate a client +secret, and generate a private key (`.pem`). + +The webhook must reach the server over public HTTPS. Without it, pull requests +update only at server startup and on Resync. + +## Configure the server + +Set every variable; a partial set stops the server with the missing names. +Secrets are read only from the environment. + +| Variable | Value | +| -------------------------------------- | ------------------------------------------------------------------------------------ | +| `DIFFX_SERVER_GITHUB_APP_ID` | App ID | +| `DIFFX_SERVER_GITHUB_CLIENT_ID` | Client ID | +| `DIFFX_SERVER_GITHUB_CLIENT_SECRET` | Client secret | +| `DIFFX_SERVER_GITHUB_PRIVATE_KEY_FILE` | Path to the private key file | +| `DIFFX_SERVER_GITHUB_WEBHOOK_SECRET` | Webhook secret | +| `DIFFX_SERVER_GITHUB_PUBLIC_URL` | Browser-facing HTTPS origin, such as `https://diffx.example.com` | +| `DIFFX_SERVER_GITHUB_TOKEN_KEY` | 32 random bytes in base64 (`openssl rand -base64 32`); encrypts stored GitHub tokens | +| `DIFFX_SERVER_FETCHER_SECRET` | At least 32 characters (`openssl rand -hex 32`); shared with the fetcher | + +The server listens for the fetcher on `DIFFX_SERVER_INTERNAL_PORT` (default 7979) on the same host as its public listener. Never publish that port; only +the fetcher should reach it. Keep `DIFFX_SERVER_GITHUB_TOKEN_KEY` stable: +changing it requires every account to connect GitHub again. + +## Run the fetcher + +With Compose, put the variables above in a git-ignored `.env` beside +`compose.yaml`, mount the private key into the server with an override file, +and start the `github` profile: + +```yaml +# compose.override.yaml +services: + server: + environment: + DIFFX_SERVER_GITHUB_PRIVATE_KEY_FILE: /run/secrets/github-app.pem + volumes: + - ./github-app.pem:/run/secrets/github-app.pem:ro +``` + +```sh +docker compose --profile github up -d +``` + +The fetcher keeps mirrors in the `diffx-mirrors` volume and evicts the least +recently used ones above `DIFFX_FETCHER_DISK_LIMIT_GB` (default 20). +`DIFFX_FETCHER_CONCURRENCY` (default 2, at most 8) sets parallel fetches. + +To run it elsewhere, build `docker build --target fetcher .` and set +`DIFFX_FETCHER_SERVER_URL` (the server's internal listener, such as +`http://server:7979`), `DIFFX_FETCHER_SECRET` and `DIFFX_FETCHER_MIRRORS`. The +fetcher needs Git 2.44 or later and HTTPS access to github.com. + +## Connect and follow + +1. Open **Account settings** and choose **Connect GitHub**. GitHub asks you to + authorize the app, then returns to Settings. +2. Choose **Install on repositories** to install the app where you need it. + You can follow only repositories that both you and the app can read. +3. Check the repositories to follow. Their open pull requests, drafts + included, appear in the Reviews column shortly. + +**Resync pull requests** rechecks access and refreshes every followed +repository, including pull requests that closed without an event. Resync also +extends retention of pull requests that have not changed recently. If GitHub +access expires or is revoked, Settings asks you to connect again; followed +repositories receive nothing until then. **Disconnect** removes the +authorization and every follow; existing reviews stay until they expire. + +## Limits + +- A push, edit or state change creates a new snapshot in the same review + stream, so comments and reviewed marks carry across force-pushes. +- Pull request descriptions render as Markdown without raw HTML; images show + as links and are never loaded. +- Repositories cloned over SSH, or without `.git` in their HTTPS remote, group + separately from their pull requests. +- Very large pull requests can exceed collection time or preview limits. diff --git a/docs-website/vocs.config.ts b/docs-website/vocs.config.ts index 052b352d..aa8b239a 100644 --- a/docs-website/vocs.config.ts +++ b/docs-website/vocs.config.ts @@ -19,6 +19,7 @@ export default defineConfig({ text: "Agents and remote", items: [ { text: "Agent hooks and remote ingestion", link: "/agent-hooks" }, + { text: "GitHub pull requests", link: "/github-pull-requests" }, { text: "Agent plugins", link: "/plugins" }, { text: "MCP", link: "/mcp" }, { text: "WebMCP", link: "/webmcp" }, diff --git a/docs/development.md b/docs/development.md index 26e95f92..3c452d2e 100644 --- a/docs/development.md +++ b/docs/development.md @@ -376,6 +376,12 @@ repository grouping. Unavailable entries are disabled and skipped during keyboar navigation. Filter selections persist while navigating and reopening the picker. Switching disposes the previous workspace's request ownership and resets transient state. +GitHub pull request streams read `observation.pullRequest`, which the server +projects from the stream head: `project-nav.ts` owns their title, metadata and +the rule that closed and merged pull requests leave current work; +`PullRequestBanner.tsx` renders the banner; `account-github.tsx` owns Connect, +follows and Resync in Settings and renders nothing when +`GET /api/v2/github/connection` returns 404. The compact header exposes no snapshot delete action; theme choice lives in the status bar's Settings popover. Snapshot deletion through the API removes its stored diff scopes, comments, and reviewed-file marks. diff --git a/docs/github-live-testing.md b/docs/github-live-testing.md new file mode 100644 index 00000000..806a967b --- /dev/null +++ b/docs/github-live-testing.md @@ -0,0 +1,75 @@ +# GitHub integration: pending live testing + +The GitHub pull request integration shipped without a real GitHub App. Every +GitHub call was exercised against fakes; nothing has talked to github.com with +real credentials. This note lists what remains to verify once an app exists, +and the known risks to watch. Remove it when live testing is done. + +User setup: [GitHub pull requests](../docs-website/src/pages/github-pull-requests.md). +Design: `docs/system.md` → GitHub pull requests. + +## Verified without GitHub + +- Webhook HMAC, delivery deduplication and coalesced refresh intent + (`internal/githubsync`, real binary with signed payloads). +- OAuth state binding to the starting account, sealed tokens, refresh + rotation and reconnect on rejection (fake GitHub). +- Lease fencing, SHA checks, backoff, fan-out admission in one transaction, + deterministic submission IDs and Resync freshness (`fetch_test.go`). +- Fetcher partial clone, offline collection, token never persisted, LRU + eviction (`internal/fetcher`, local `file://` origin). +- Real git origin → fetcher → lease/finish → stored review with pull request + metadata (`internal/fetcher/integration_test.go`). +- Internal listener secret and isolation from the public listener (real + binaries); fetcher and server images build. +- Settings, Reviews column and banner (browser tests with mocked API). + +## Not yet verified + +Each needs a real GitHub App, an installation and public HTTPS for webhooks +(or `gh webhook forward` / smee during development). + +1. **App JWT and installation tokens.** `GET /app` (slug), repository-scoped + `POST /app/installations/{id}/access_tokens` with `contents: read`. +2. **OAuth web flow.** Authorize redirect, callback, code exchange with + expiring user tokens, `GET /user`, `GET /user/installations` and its + repositories. Confirm the callback lands on `/settings?github=connected`. +3. **Token refresh.** Let a user token expire (8h) and confirm refresh, + rotation and storage; revoke the app authorization on GitHub and confirm + Settings asks to connect again. +4. **Webhooks.** `pull_request` opened, synchronize (push and force-push), + edited, ready_for_review, converted_to_draft, closed and merged; + `installation` deleted and suspended; `installation_repositories` removed; + `github_app_authorization` revoked. Confirm redelivery from GitHub's UI is + a no-op. +5. **Fetch over HTTPS.** `x-access-token` basic auth through + `http.extraHeader`, partial clone of `refs/pull/N/head`, blob prefetch with + `fetch --stdin`, for a same-repository and a fork pull request, public and + private. +6. **End to end in Compose.** `docker compose --profile github up`: fetcher + reaches `server:7979` on the `fetch` network; 7979 is not published; the + pull request appears in the Reviews column grouped with an HTTPS checkout of + the same repository; the banner shows author, branches and description. +7. **Fan-out.** Two accounts following one repository both receive the pull + request; an account without access does not. +8. **Reconcile.** Close a pull request while the server is stopped; on startup + it leaves the Reviews column. Resync extends retention of an idle one. +9. **Rate limits.** Behavior when GitHub returns 403/429 rate limiting during + listing, lease or Resync. + +## Known risks and likely follow-ups + +- The collector's 30-second timeout may be too short for very large pull + requests; the fetcher may need its own budget. +- The fetcher passes no proxy or CA environment to Git, so deployments behind + an HTTPS proxy fail. +- A corrupt mirror is not repaired automatically; delete it from the + `diffx-mirrors` volume to rebuild it. +- Disk usage is checked after each job, so concurrent fetches can briefly + exceed `DIFFX_FETCHER_DISK_LIMIT_GB`. +- Listing caps (10,000 repositories, 3,000 open pull requests per repository) + truncate silently. +- Checkouts cloned over SSH or without `.git` in their HTTPS remote group + separately from their pull requests. +- Each lease reads the pull request and mints a token; heavy webhook traffic + on large repositories may need caching of installation tokens per lease. diff --git a/docs/system.md b/docs/system.md index 913ea280..2da108e1 100644 --- a/docs/system.md +++ b/docs/system.md @@ -126,6 +126,9 @@ freshness follows collection time and acceptance sequence. ### GitHub pull requests +Live testing against a real GitHub App is pending; see +[github-live-testing.md](github-live-testing.md). + The remote server optionally integrates a GitHub App, enabled only when every `DIFFX_SERVER_GITHUB_*` setting is present; secrets come from the environment only. Local composition has no GitHub routes. diff --git a/packages/api/src/client.ts b/packages/api/src/client.ts index e1ee0798..d1aa5137 100644 --- a/packages/api/src/client.ts +++ b/packages/api/src/client.ts @@ -22,6 +22,9 @@ export type ApiReviewComment = components["schemas"]["ReviewComment"]; export type ApiReviewMark = components["schemas"]["ReviewMark"]; export type ApiCollectionResult = components["schemas"]["CollectionResult"]; export type ApiCollectionFailure = components["schemas"]["CollectionFailure"]; +export type ApiPullRequest = components["schemas"]["PullRequest"]; +export type ApiGitHubConnection = components["schemas"]["GitHubConnection"]; +export type ApiGitHubRepository = components["schemas"]["GitHubRepository"]; export function errorDetail(error: unknown, fallback: string) { if (error instanceof Error) return error.message; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3ce2edb7..f60b719c 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -60,6 +60,12 @@ catalogs: react-dom: specifier: 19.2.8 version: 19.2.8 + react-markdown: + specifier: 10.1.0 + version: 10.1.0 + remark-gfm: + specifier: 4.0.1 + version: 4.0.1 shadcn: specifier: 4.21.0 version: 4.21.0 @@ -137,6 +143,12 @@ importers: react-dom: specifier: 'catalog:' version: 19.2.8(react@19.2.8) + react-markdown: + specifier: 'catalog:' + version: 10.1.0(@types/react@19.2.18)(react@19.2.8)(supports-color@10.2.2) + remark-gfm: + specifier: 'catalog:' + version: 4.0.1(supports-color@10.2.2) shadcn: specifier: 'catalog:' version: 4.21.0(supports-color@10.2.2)(typescript@7.0.2) @@ -3750,6 +3762,9 @@ packages: resolution: {integrity: sha512-Hc+ghLoSt6QaYZUv0WBiIvmMDZuZZ7oaDvdH8MbfOO4lOsxdXLEvuC6ePoGs9H1X9oCLyq6+NVN0MKqD+ydxyg==} engines: {node: ^20.17.0 || >=22.9.0} + html-url-attributes@3.0.1: + resolution: {integrity: sha512-ol6UPyBWqsrO6EJySPz2O7ZSr856WDrEzM5zMqp+FJJLGMW35cLYmmZnl0vztAZxRUoNZJFTCohfjuIJ8I4QBQ==} + html-void-elements@3.0.0: resolution: {integrity: sha512-bEqo66MRXsUGxWHV5IP0PUiAWwoEjba4VCzg0LjFJBpchPaTfyfCKTG6bc5F8ucKec3q5y6qOdGyYTSBEvhCrg==} @@ -5145,6 +5160,12 @@ packages: react-is@17.0.2: resolution: {integrity: sha512-w2GsyukL62IJnlaff/nRegPQR94C/XXamvMWmSHRJ4y7Ts/4ocGRmTHvOs8PSE6pB3dWOrD/nueuU5sduBsQ4w==} + react-markdown@10.1.0: + resolution: {integrity: sha512-qKxVopLT/TyA6BX3Ue5NwabOsAzm0Q7kAPwq6L+wWDwisYs7R8vZ0nRXqq6rkueboxpkjvLGU9fWifiX/ZZFxQ==} + peerDependencies: + '@types/react': '>=18' + react: '>=18' + react-server-dom-webpack@19.3.1: resolution: {integrity: sha512-I1Smf6Yc+389rFWPwyYjvCQVK1XFWpb2M9gvgYgfpbZ+LPqkZtb0tT9Fwdk0K+wknK2E+ns82lC0DCYLnXCVGA==} engines: {node: '>=0.10.0'} @@ -10150,6 +10171,8 @@ snapshots: dependencies: lru-cache: 11.5.3 + html-url-attributes@3.0.1: {} + html-void-elements@3.0.0: {} html-whitespace-sensitive-tag-names@3.0.1: {} @@ -10835,7 +10858,7 @@ snapshots: micromark-util-character: 2.1.1 micromark-util-sanitize-uri: 2.0.1 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-extension-gfm-footnote@2.1.0: dependencies: @@ -10846,7 +10869,7 @@ snapshots: micromark-util-normalize-identifier: 2.0.1 micromark-util-sanitize-uri: 2.0.1 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-extension-gfm-strikethrough@2.1.0: dependencies: @@ -10855,7 +10878,7 @@ snapshots: micromark-util-classify-character: 2.0.1 micromark-util-resolve-all: 2.0.1 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-extension-gfm-table@2.1.2: dependencies: @@ -10863,11 +10886,11 @@ snapshots: micromark-factory-space: 2.1.0 micromark-util-character: 2.1.1 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-extension-gfm-tagfilter@2.0.0: dependencies: - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-extension-gfm-task-list-item@2.1.0: dependencies: @@ -10875,7 +10898,7 @@ snapshots: micromark-factory-space: 2.1.0 micromark-util-character: 2.1.1 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-extension-gfm@3.0.0: dependencies: @@ -10886,7 +10909,7 @@ snapshots: micromark-extension-gfm-tagfilter: 2.0.0 micromark-extension-gfm-task-list-item: 2.1.0 micromark-util-combine-extensions: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-extension-mdx-expression@3.0.1: dependencies: @@ -10897,7 +10920,7 @@ snapshots: micromark-util-character: 2.1.1 micromark-util-events-to-acorn: 2.0.3 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-extension-mdx-jsx@3.0.2: dependencies: @@ -10909,12 +10932,12 @@ snapshots: micromark-util-character: 2.1.1 micromark-util-events-to-acorn: 2.0.3 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 vfile-message: 4.0.3 micromark-extension-mdx-md@2.0.0: dependencies: - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-extension-mdxjs-esm@3.0.0: dependencies: @@ -10924,7 +10947,7 @@ snapshots: micromark-util-character: 2.1.1 micromark-util-events-to-acorn: 2.0.3 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 unist-util-position-from-estree: 2.0.0 vfile-message: 4.0.3 @@ -10943,14 +10966,14 @@ snapshots: dependencies: micromark-util-character: 2.1.1 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-factory-label@2.0.1: dependencies: devlop: 1.1.0 micromark-util-character: 2.1.1 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-factory-mdx-expression@2.0.3: dependencies: @@ -10960,28 +10983,28 @@ snapshots: micromark-util-character: 2.1.1 micromark-util-events-to-acorn: 2.0.3 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 unist-util-position-from-estree: 2.0.0 vfile-message: 4.0.3 micromark-factory-space@2.1.0: dependencies: micromark-util-character: 2.1.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-factory-title@2.0.1: dependencies: micromark-factory-space: 2.1.0 micromark-util-character: 2.1.1 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-factory-whitespace@2.0.1: dependencies: micromark-factory-space: 2.1.0 micromark-util-character: 2.1.1 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-util-character@2.1.1: dependencies: @@ -11001,7 +11024,7 @@ snapshots: micromark-util-combine-extensions@2.0.1: dependencies: micromark-util-chunked: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-util-decode-numeric-character-reference@2.0.2: dependencies: @@ -11016,7 +11039,7 @@ snapshots: micromark-util-edit-map@1.0.0: dependencies: - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-util-encode@2.0.1: {} @@ -11027,7 +11050,7 @@ snapshots: devlop: 1.1.0 estree-util-visit: 2.0.0 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 vfile-message: 4.0.3 micromark-util-html-tag-name@2.0.1: {} @@ -11038,7 +11061,7 @@ snapshots: micromark-util-resolve-all@2.0.1: dependencies: - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-util-sanitize-uri@2.0.1: dependencies: @@ -11051,7 +11074,7 @@ snapshots: devlop: 1.1.0 micromark-util-chunked: 2.0.1 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 micromark-util-symbol@2.0.1: {} @@ -11078,7 +11101,7 @@ snapshots: micromark-util-sanitize-uri: 2.0.1 micromark-util-subtokenize: 2.1.0 micromark-util-symbol: 2.0.1 - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 transitivePeerDependencies: - supports-color @@ -11723,6 +11746,24 @@ snapshots: react-is@17.0.2: {} + react-markdown@10.1.0(@types/react@19.2.18)(react@19.2.8)(supports-color@10.2.2): + dependencies: + '@types/hast': 3.0.5 + '@types/mdast': 4.0.4 + '@types/react': 19.2.18 + devlop: 1.1.0 + hast-util-to-jsx-runtime: 2.3.6(supports-color@10.2.2) + html-url-attributes: 3.0.1 + mdast-util-to-hast: 13.2.1 + react: 19.2.8 + remark-parse: 11.0.0(supports-color@10.2.2) + remark-rehype: 11.1.2 + unified: 11.0.5 + unist-util-visit: 5.1.0 + vfile: 6.0.3 + transitivePeerDependencies: + - supports-color + react-server-dom-webpack@19.3.1(react-dom@19.3.1(react@19.3.1))(react@19.3.1)(webpack@5.112.0(esbuild@0.28.2)(lightningcss@1.33.0)(postcss@8.5.30)): dependencies: acorn-loose: 8.5.2 @@ -11907,7 +11948,7 @@ snapshots: dependencies: '@types/mdast': 4.0.4 mdast-util-from-markdown: 2.1.0(supports-color@10.2.2) - micromark-util-types: 2.0.2 + micromark-util-types: 2.0.3 unified: 11.0.5 transitivePeerDependencies: - supports-color diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index a1ab2000..407de088 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -24,6 +24,8 @@ catalog: class-variance-authority: 0.7.1 cn: 0.2.6 lucide-react: 1.43.0 + react-markdown: 10.1.0 + remark-gfm: 4.0.1 shadcn: 4.21.0 tw-animate-css: 1.4.0 webmcp-types: 0.1.5