From 61ab7645abab3026d501a899bc588fee353dcceb Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Fri, 21 Aug 2026 11:28:02 +0200 Subject: [PATCH] feat(github): submit one review instead of posting comments one at a time Pushing looped POST /pulls/:n/comments, so a review arrived as N separate comments and N notifications, a failure part-way left half a review on the pull request, and there was nowhere for a summary to live. Threads with replies were silently unpushable, and general comments were dropped entirely. createReview submits POST /pulls/:n/reviews once: an event, a summary body, and every comment. Comments already on the pull request are still filtered first, since GitHub does not deduplicate. The dialog now selects what goes out. Everything open starts selected, deselecting is remembered against later arrivals from the agent, a thread's replies are folded into the one comment GitHub will hold, and general comments seed the summary rather than being discarded. Approve and Request changes are offered but disabled on your own pull request, which GitHub refuses - authorship comes from comparing the PR author with the authenticated login, as gh has no viewerDidAuthor field. Removes pushComments, which nothing calls now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- packages/cli/src/server.ts | 19 +- packages/github/src/detection.ts | 20 +- packages/github/src/index.ts | 4 +- packages/github/src/pr.ts | 115 +++++--- packages/github/src/types.ts | 18 ++ .../src/components/layout/github-dialog.tsx | 261 ++++++++++++++---- packages/ui/src/lib/api.ts | 28 +- packages/ui/src/lib/review-submission.ts | 41 +++ packages/ui/tests/review-submission.test.ts | 107 +++++++ 9 files changed, 493 insertions(+), 120 deletions(-) create mode 100644 packages/ui/src/lib/review-submission.ts create mode 100644 packages/ui/tests/review-submission.test.ts diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index 0b6ff91..15637f5 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -41,9 +41,10 @@ import { import { detectRemote as detectGitHubRemote, fetchDetails as fetchGitHubDetails, - pushComments as pushGitHubComments, + createReview as createGitHubReview, pullComments as pullGitHubComments, type PrComment, + type ReviewEvent, } from '@diffity/github'; import { findOrCreateSession } from './session.js'; import { createThread, addReply, getThreadsForSession } from './threads.js'; @@ -78,6 +79,8 @@ const MIME_TYPES: Record = { * `script-src` still needs 'unsafe-inline' for the inline scripts in the built index.html; * markdown is sanitized separately, and every exfiltration sink is closed here. */ +const REVIEW_EVENTS = new Set(['COMMENT', 'APPROVE', 'REQUEST_CHANGES']); + const CONTENT_SECURITY_POLICY = [ "default-src 'self'", "base-uri 'none'", @@ -497,7 +500,7 @@ export function startServer(options: ServerOptions): Promise { return; } - if (pathname === '/api/github/push-comments' && req.method === 'POST') { + if (pathname === '/api/github/create-review' && req.method === 'POST') { const details = githubRemote ? fetchGitHubDetails(githubRemote.owner, githubRemote.repo) : null; if (!githubRemote || !details?.headSha) { sendError(res, 400, 'No GitHub PR detected'); @@ -513,17 +516,19 @@ export function startServer(options: ServerOptions): Promise { return; } const body = JSON.parse(await readBody(req)); - const comments = body.comments as PrComment[]; - if (!Array.isArray(comments) || comments.length === 0) { - sendError(res, 400, 'No comments provided'); + const comments = (body.comments ?? []) as PrComment[]; + const summary = typeof body.body === 'string' ? body.body : ''; + const event = REVIEW_EVENTS.has(body.event) ? (body.event as ReviewEvent) : 'COMMENT'; + if (!Array.isArray(comments) || (comments.length === 0 && !summary.trim())) { + sendError(res, 400, 'A review needs a summary or at least one comment'); return; } - const result = pushGitHubComments( + const result = createGitHubReview( githubRemote.owner, githubRemote.repo, details.prNumber, details.headSha, - comments, + { event, body: summary, comments }, ); sendJson(res, result); return; diff --git a/packages/github/src/detection.ts b/packages/github/src/detection.ts index 9741272..4fcbf43 100644 --- a/packages/github/src/detection.ts +++ b/packages/github/src/detection.ts @@ -49,6 +49,7 @@ export function fetchDetails(owner: string, repo: string): GitHubDetails | null prCreatedAt: pr.createdAt, headSha: pr.headSha, commentCount, + viewerDidAuthor: !!pr.authorLogin && pr.authorLogin === getViewerLogin(), }; } @@ -58,11 +59,27 @@ interface PrData { url: string; headSha: string; createdAt: string; + authorLogin: string | null; +} + +// gh has no `viewerDidAuthor` field, so authorship is settled by comparing logins. The +// authenticated user cannot change while the process lives, so it is asked for once. +let viewerLogin: string | null | undefined; + +function getViewerLogin(): string | null { + if (viewerLogin === undefined) { + try { + viewerLogin = exec('gh api user --jq .login') || null; + } catch { + viewerLogin = null; + } + } + return viewerLogin; } function getPr(): PrData | null { try { - const json = exec('gh pr view --json number,title,url,headRefOid,createdAt'); + const json = exec('gh pr view --json number,title,url,headRefOid,createdAt,author'); const data = JSON.parse(json); if (data.number && data.url && data.headRefOid) { return { @@ -71,6 +88,7 @@ function getPr(): PrData | null { url: data.url, headSha: data.headRefOid, createdAt: data.createdAt, + authorLogin: data.author?.login ?? null, }; } return null; diff --git a/packages/github/src/index.ts b/packages/github/src/index.ts index 39e849c..e829d17 100644 --- a/packages/github/src/index.ts +++ b/packages/github/src/index.ts @@ -1,4 +1,4 @@ -export type { GitHubRemote, GitHubDetails, PrBase, PrComment, PushResult, PulledThread } from './types.js'; +export type { GitHubRemote, GitHubDetails, PrBase, PrComment, PulledThread, ReviewEvent, ReviewResult, ReviewSubmission } from './types.js'; export { detectRemote, fetchDetails, isCliInstalled, isAuthenticated } from './detection.js'; -export { getFiles, getComments, getCommentCount, pushComments, pullComments } from './pr.js'; +export { getFiles, getComments, getCommentCount, pullComments, createReview } from './pr.js'; export { isGitHubPrUrl, parseGitHubPrUrl, checkoutPr, getPrBase, parsePrBase } from './pr-url.js'; diff --git a/packages/github/src/pr.ts b/packages/github/src/pr.ts index f2c0c18..967c5f2 100644 --- a/packages/github/src/pr.ts +++ b/packages/github/src/pr.ts @@ -1,6 +1,6 @@ -import { execSync } from 'node:child_process'; +import { execFileSync, execSync } from 'node:child_process'; import { exec } from './exec.js'; -import type { PrComment, PushResult, PulledThread } from './types.js'; +import type { PrComment, PulledThread, ReviewResult, ReviewSubmission } from './types.js'; export function getFiles(owner: string, repo: string, prNumber: number): Set { try { @@ -114,61 +114,98 @@ function isDuplicate(existing: ExistingComment[], comment: PrComment): boolean { ); } -export function pushComments( +interface ReviewCommentPayload { + path: string; + side: string; + line: number; + body: string; + start_line?: number; + start_side?: string; +} + +function toReviewComment(comment: PrComment): ReviewCommentPayload { + const payload: ReviewCommentPayload = { + path: comment.filePath, + side: comment.side, + line: comment.endLine, + body: comment.body, + }; + + if (comment.startLine && comment.startLine !== comment.endLine) { + payload.start_line = comment.startLine; + payload.start_side = comment.side; + } + + return payload; +} + +/** + * Submits one review holding every comment, rather than posting them one at a time: the author + * gets a single notification, a partial failure cannot leave half a review on the pull request, + * and the summary body has somewhere to live. + * + * GitHub does not deduplicate, so comments already on the pull request are dropped first. + */ +export function createReview( owner: string, repo: string, prNumber: number, headSha: string, - comments: PrComment[], -): PushResult { + submission: ReviewSubmission, +): ReviewResult { const prFiles = getFiles(owner, repo, prNumber); const existing = getComments(owner, repo, prNumber); - let pushed = 0; - let skipped = 0; - let failed = 0; const errors: string[] = []; + const comments: ReviewCommentPayload[] = []; + let skipped = 0; - for (const comment of comments) { + for (const comment of submission.comments) { if (!prFiles.has(comment.filePath)) { - failed++; errors.push(`${comment.filePath} — not in PR diff (push your changes first)`); continue; } - if (isDuplicate(existing, comment)) { skipped++; continue; } + comments.push(toReviewComment(comment)); + } - try { - const payload: Record = { - body: comment.body, - commit_id: headSha, - path: comment.filePath, - side: comment.side, - line: comment.endLine, - }; - if (comment.startLine && comment.startLine !== comment.endLine) { - payload.start_line = comment.startLine; - payload.start_side = comment.side; - } - execSync( - `gh api repos/${owner}/${repo}/pulls/${prNumber}/comments --method POST --input -`, - { - input: JSON.stringify(payload), - encoding: 'utf-8', - stdio: 'pipe', - }, - ); - pushed++; - } catch (err) { - failed++; - const msg = err instanceof Error ? err.message : String(err); - const ghLine = msg.split('\n').find(l => l.includes('gh:')); - errors.push(`${comment.filePath}:${comment.endLine} — ${ghLine ? ghLine.trim() : 'GitHub API error'}`); - } + const dropped = errors.length; + const body = submission.body.trim(); + + if (comments.length === 0 && !body) { + return { submitted: 0, skipped, failed: dropped, errors, reviewUrl: null }; } - return { pushed, skipped, failed, errors }; + try { + const raw = execFileSync( + 'gh', + ['api', `repos/${owner}/${repo}/pulls/${prNumber}/reviews`, '--method', 'POST', '--input', '-'], + { + input: JSON.stringify({ commit_id: headSha, event: submission.event, body, comments }), + encoding: 'utf-8', + stdio: 'pipe', + }, + ); + const review = JSON.parse(raw) as { html_url?: string }; + return { + submitted: comments.length, + skipped, + failed: dropped, + errors, + reviewUrl: review.html_url ?? null, + }; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + const ghLine = msg.split('\n').find(line => line.includes('gh:')); + return { + submitted: 0, + skipped, + failed: dropped + comments.length, + errors: [...errors, ghLine ? ghLine.trim() : 'GitHub rejected the review'], + reviewUrl: null, + }; + } } diff --git a/packages/github/src/types.ts b/packages/github/src/types.ts index dc77850..5723bf3 100644 --- a/packages/github/src/types.ts +++ b/packages/github/src/types.ts @@ -10,6 +10,24 @@ export interface GitHubDetails { prCreatedAt: string; headSha: string; commentCount: number; + /** GitHub refuses to approve or request changes on your own pull request. */ + viewerDidAuthor: boolean; +} + +export type ReviewEvent = 'COMMENT' | 'APPROVE' | 'REQUEST_CHANGES'; + +export interface ReviewSubmission { + event: ReviewEvent; + body: string; + comments: PrComment[]; +} + +export interface ReviewResult { + submitted: number; + skipped: number; + failed: number; + errors: string[]; + reviewUrl: string | null; } export interface PrBase { diff --git a/packages/ui/src/components/layout/github-dialog.tsx b/packages/ui/src/components/layout/github-dialog.tsx index d09cc3e..a3ed7cd 100644 --- a/packages/ui/src/components/layout/github-dialog.tsx +++ b/packages/ui/src/components/layout/github-dialog.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect } from 'react'; +import { useState, useEffect, useMemo } from 'react'; import dayjs from 'dayjs'; import relativeTime from 'dayjs/plugin/relativeTime'; import { toast } from 'sonner'; @@ -6,9 +6,18 @@ import { GitHubIcon } from '../icons/github-icon'; import { UploadIcon } from '../icons/upload-icon'; import { DownloadIcon } from '../icons/download-icon'; import { XIcon } from '../icons/x-icon'; -import { pushCommentsToGitHub, pullCommentsFromGitHub, type GitHubDetails, type PrCommentPayload } from '../../lib/api'; +import { + createReviewOnGitHub, + pullCommentsFromGitHub, + type GitHubDetails, + type ReviewEvent, +} from '../../lib/api'; import type { CommentThread } from '../comments/types'; -import { GENERAL_THREAD_FILE_PATH, isThreadResolved } from '../comments/types'; +import { + isSubmittable, + summaryFromGeneralThreads, + threadToPayload, +} from '../../lib/review-submission'; dayjs.extend(relativeTime); @@ -20,18 +29,49 @@ interface GitHubDialogProps { onClose: () => void; } +const EVENT_LABELS: Record = { + COMMENT: 'Comment', + APPROVE: 'Approve', + REQUEST_CHANGES: 'Request changes', +}; + +function lineLabel(thread: CommentThread): string { + return thread.startLine === thread.endLine + ? `${thread.startLine}` + : `${thread.startLine}-${thread.endLine}`; +} + export function GitHubDialog(props: GitHubDialogProps) { const { details, threads, sessionId, onPulled, onClose } = props; const [commentCount, setCommentCount] = useState(details.commentCount); - const [pushing, setPushing] = useState(false); + const [submitting, setSubmitting] = useState(false); const [pulling, setPulling] = useState(false); + const [event, setEvent] = useState('COMMENT'); - const unresolvedFileThreads = threads.filter( - t => !isThreadResolved(t) && t.filePath !== GENERAL_THREAD_FILE_PATH, - ); - const pushableThreads = unresolvedFileThreads.filter(t => t.comments.length === 1); - const multiCommentCount = unresolvedFileThreads.length - pushableThreads.length; - const localCount = unresolvedFileThreads.length; + const submittable = useMemo(() => threads.filter(isSubmittable), [threads]); + const [selected, setSelected] = useState>(new Set()); + const [summary, setSummary] = useState(''); + const [summaryEdited, setSummaryEdited] = useState(false); + + // Everything open is selected by default, including threads arriving from the agent while + // the dialog is open; deselecting is the deliberate act. + useEffect(() => { + setSelected(prev => { + const next = new Set(prev); + for (const thread of submittable) { + if (!prev.has(`-${thread.id}`)) { + next.add(thread.id); + } + } + return next; + }); + }, [submittable]); + + useEffect(() => { + if (!summaryEdited) { + setSummary(summaryFromGeneralThreads(threads)); + } + }, [threads, summaryEdited]); useEffect(() => { const handleKey = (e: KeyboardEvent) => { @@ -43,38 +83,60 @@ export function GitHubDialog(props: GitHubDialogProps) { return () => window.removeEventListener('keydown', handleKey); }, [onClose]); - const handlePush = async () => { - if (pushableThreads.length === 0) { + const chosen = submittable.filter(thread => selected.has(thread.id)); + const canSubmit = chosen.length > 0 || summary.trim().length > 0; + + const toggle = (id: string) => { + setSelected(prev => { + const next = new Set(prev); + if (next.has(id)) { + next.delete(id); + // Remembered so the default-select effect does not re-add it. + next.add(`-${id}`); + } else { + next.delete(`-${id}`); + next.add(id); + } + return next; + }); + }; + + const toggleAll = () => { + if (chosen.length === submittable.length) { + setSelected(new Set(submittable.map(thread => `-${thread.id}`))); return; } - setPushing(true); + setSelected(new Set(submittable.map(thread => thread.id))); + }; + + const handleSubmit = async () => { + if (!canSubmit) { + return; + } + setSubmitting(true); try { - const comments: PrCommentPayload[] = pushableThreads.map(t => ({ - filePath: t.filePath, - side: t.side === 'old' ? 'LEFT' as const : 'RIGHT' as const, - startLine: t.startLine !== t.endLine ? t.startLine : null, - endLine: t.endLine, - body: t.comments[0].body, - })); - const result = await pushCommentsToGitHub(comments); + const result = await createReviewOnGitHub({ + event, + body: summary, + comments: chosen.map(threadToPayload), + }); + if (result.failed > 0) { - const pushedMsg = result.pushed > 0 ? `${result.pushed} pushed, ` : ''; - toast.error(`${pushedMsg}${result.failed} failed`, { + toast.error(`Review not submitted — ${result.failed} comment${result.failed !== 1 ? 's' : ''} rejected`, { description: result.errors.join('\n'), }); - } else if (result.pushed === 0 && result.skipped > 0) { - toast.info('All comments already exist on the PR'); } else { - const skippedMsg = result.skipped > 0 ? ` (${result.skipped} already existed)` : ''; - toast.success(`Pushed ${result.pushed} comment${result.pushed !== 1 ? 's' : ''} to PR${skippedMsg}`); + const skipped = result.skipped > 0 ? ` (${result.skipped} already on the PR)` : ''; + toast.success(`Submitted ${result.submitted} comment${result.submitted !== 1 ? 's' : ''} as one review${skipped}`); + setCommentCount(prev => prev + result.submitted); + onClose(); } - setCommentCount(prev => prev + result.pushed); } catch (err) { - toast.error('Failed to push comments', { + toast.error('Failed to submit review', { description: err instanceof Error ? err.message : 'Unknown error', }); } finally { - setPushing(false); + setSubmitting(false); } }; @@ -103,7 +165,7 @@ export function GitHubDialog(props: GitHubDialogProps) { return (
e.stopPropagation()} >
@@ -126,40 +188,117 @@ export function GitHubDialog(props: GitHubDialogProps) {
-
-
-
-
- {pushableThreads.length > 0 - ? `${pushableThreads.length} comment${pushableThreads.length !== 1 ? 's' : ''} to push` - : `${localCount} local comment${localCount !== 1 ? 's' : ''}`} -
-
- {pushableThreads.length > 0 && multiCommentCount > 0 - ? `${multiCommentCount} thread${multiCommentCount !== 1 ? 's' : ''} with replies can't be pushed` - : pushableThreads.length > 0 - ? 'Single comments, ready to push' - : localCount > 0 - ? 'Threads with replies can\'t be pushed' - : 'No comments to push'} -
+
+
+
+ + Summary + + Markdown
- {pushableThreads.length > 0 && ( - +