diff --git a/packages/ui/src/components/layout/github-dialog.tsx b/packages/ui/src/components/layout/github-dialog.tsx index acddcec..3d0ce2a 100644 --- a/packages/ui/src/components/layout/github-dialog.tsx +++ b/packages/ui/src/components/layout/github-dialog.tsx @@ -263,7 +263,7 @@ export function GitHubDialog(props: GitHubDialogProps) { )} {thread.comments.length > 1 && ( - +{thread.comments.length - 1} repl{thread.comments.length === 2 ? 'y' : 'ies'} + +{thread.comments.length - 1} repl{thread.comments.length === 2 ? 'y' : 'ies'} · not sent )} diff --git a/packages/ui/src/lib/review-submission.ts b/packages/ui/src/lib/review-submission.ts index d8c8108..42e564d 100644 --- a/packages/ui/src/lib/review-submission.ts +++ b/packages/ui/src/lib/review-submission.ts @@ -2,21 +2,21 @@ import type { CommentKind, CommentThread } from '../components/comments/types'; import { GENERAL_THREAD_FILE_PATH, isThreadResolved } from '../components/comments/types'; import type { PrCommentPayload, ReviewEvent } from './api'; -/** - * A thread's replies are part of the same finding, so they are folded into the one comment - * GitHub will hold — a review comment has no thread of its own until it exists. - */ export function isReviewComment(comment: { kind?: CommentKind }): boolean { return (comment.kind ?? 'review') === 'review'; } +/** + * Only the finding travels. Everything said after it was said to work out what the finding should + * say, so the way to get an answer onto the pull request is to amend the finding, not to ship the + * conversation that produced it. + */ +export function findingOf(thread: CommentThread): string | undefined { + return thread.comments.find(isReviewComment)?.body; +} + export function threadToPayload(thread: CommentThread): PrCommentPayload { - // An aside is a conversation with the agent about the review, not part of it. Sending one would - // put the whole exchange on the pull request. - const [first, ...replies] = thread.comments.filter(isReviewComment); - const body = replies.length - ? [first.body, ...replies.map(reply => `**${reply.author.name}:** ${reply.body}`)].join('\n\n---\n\n') - : first.body; + const finding = findingOf(thread); return { threadId: thread.id, @@ -24,7 +24,7 @@ export function threadToPayload(thread: CommentThread): PrCommentPayload { side: thread.side === 'old' ? 'LEFT' : 'RIGHT', startLine: thread.startLine !== thread.endLine ? thread.startLine : null, endLine: thread.endLine, - body, + body: finding ?? '', }; } @@ -47,7 +47,8 @@ export function isGeneral(thread: CommentThread): boolean { export function summaryFromGeneralThreads(threads: CommentThread[]): string { return threads .filter(isGeneral) - .flatMap(thread => thread.comments.map(comment => comment.body)) + .map(findingOf) + .filter(body => !!body) .join('\n\n'); } diff --git a/packages/ui/tests/review-submission.test.ts b/packages/ui/tests/review-submission.test.ts index e3f1e85..173a77e 100644 --- a/packages/ui/tests/review-submission.test.ts +++ b/packages/ui/tests/review-submission.test.ts @@ -49,14 +49,14 @@ describe('threadToPayload', () => { expect(threadToPayload(thread({ side: 'old' })).side).toBe('LEFT'); }); - it('folds replies into one body, attributed', () => { + it('sends the finding alone, however much was said after it', () => { const payload = threadToPayload( thread({ comments: [comment('P2: name is unclear'), comment('agreed, renaming', 'Agent')], }), ); - expect(payload.body).toBe('P2: name is unclear\n\n---\n\n**Agent:** agreed, renaming'); + expect(payload.body).toBe('P2: name is unclear'); }); }); @@ -102,6 +102,34 @@ describe('summaryFromGeneralThreads', () => { expect(summaryFromGeneralThreads([thread()])).toBe(''); }); + it('leaves the conversation about a summary out of it', () => { + const summary = summaryFromGeneralThreads([ + thread({ + filePath: GENERAL_THREAD_FILE_PATH, + comments: [ + { ...comment('Verdict: two findings, both small'), kind: 'review' as const }, + { ...comment('why only two?', 'You'), kind: 'aside' as const }, + { ...comment('because the third turned out to be mine', 'Agent'), kind: 'aside' as const }, + { ...comment('a reply nobody folded in', 'Agent'), kind: 'review' as const }, + ], + }), + ]); + + expect(summary).toBe('Verdict: two findings, both small'); + }); + + it('contributes nothing from a thread that is only a question', () => { + const summary = summaryFromGeneralThreads([ + thread({ + filePath: GENERAL_THREAD_FILE_PATH, + comments: [{ ...comment('what did you not check?', 'You'), kind: 'aside' as const }], + }), + thread({ id: 'g2', filePath: GENERAL_THREAD_FILE_PATH, comments: [comment('Verdict: fine')] }), + ]); + + expect(summary).toBe('Verdict: fine'); + }); + it('recognises a general thread', () => { expect(isGeneral(thread({ filePath: GENERAL_THREAD_FILE_PATH }))).toBe(true); expect(isGeneral(thread())).toBe(false); @@ -151,14 +179,23 @@ describe('what a thread sends to the forge', () => { } as unknown as CommentThread; } - it('sends the finding and the discussion of it', () => { + it('sends the finding and nothing else', () => { const payload = threadToPayload(withComments([ { body: 'P2: the finding' }, - { body: 'and a reply the author should see', name: 'You' }, + { body: 'a reply that was never folded into the finding', name: 'You' }, + ])); + + expect(payload.body).toBe('P2: the finding'); + }); + + it('sends an amended finding once, not the answer that produced it as well', () => { + const payload = threadToPayload(withComments([ + { body: 'P2: the finding, amended to carry the answer' }, + { body: 'is our money handling affected?', kind: 'aside', name: 'You' }, + { body: 'No, and I was wrong to point at refunds. I have amended the finding.' }, ])); - expect(payload.body).toContain('P2: the finding'); - expect(payload.body).toContain('and a reply the author should see'); + expect(payload.body).toBe('P2: the finding, amended to carry the answer'); }); // An aside is a conversation with the agent about the review. Posting it would put the whole @@ -188,7 +225,8 @@ describe('what a thread sends to the forge', () => { delete (comment as { kind?: string }).kind; } - expect(threadToPayload(thread).body).toContain('old reply'); + // Were the default an aside, there would be no finding here to send. + expect(threadToPayload(thread).body).toBe('P1: old finding'); }); });