diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index dee12e0..433f298 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'", @@ -499,7 +502,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'); @@ -515,17 +518,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 && ( - +