diff --git a/.changeset/git-multiline-review.md b/.changeset/git-multiline-review.md new file mode 100644 index 0000000..64eaad8 --- /dev/null +++ b/.changeset/git-multiline-review.md @@ -0,0 +1,15 @@ +--- +'@pdsjs/git': patch +--- + +A review comment speaks to a range of lines, not one. The +`dev.pdsjs.git.review` anchor gains `endLine` and `endSnippet`: a comment on +one line carries neither, a comment on several carries both. A reader hangs +the thread on the last line, the way a forge does, and marks the span from +the first to it. The snippets keep the range placed the way one snippet kept +a single line, since a rebase moves numbers. + +In the diff, a click on a line's gutter opens the composer on that line, and +a shift-click on another line's gutter, on the same side, makes the two ends +of a range. The rows between are marked while the comment is written and +after it is published. A comment on one line reads exactly as before. diff --git a/packages/git-ui/src/components/molecules/diff-file.jsx b/packages/git-ui/src/components/molecules/diff-file.jsx index 4156763..d913bc0 100644 --- a/packages/git-ui/src/components/molecules/diff-file.jsx +++ b/packages/git-ui/src/components/molecules/diff-file.jsx @@ -5,7 +5,7 @@ import { Badge } from '#/components/atoms/badge.jsx'; import { Button } from '#/components/atoms/button.jsx'; import { Card } from '#/components/atoms/card.jsx'; import { StatementCard } from '#/components/molecules/statement-card.jsx'; -import { lineKey } from '#/lib/comments.js'; +import { inSpan, lineKey } from '#/lib/comments.js'; import { ROW_STYLE, SIGN, SIGN_STYLE } from '#/lib/diff-rows.js'; import { bytes } from '#/lib/format.js'; import { diffHighlighter, languageFor } from '#/lib/highlight.js'; @@ -59,11 +59,12 @@ function DiffImage({ image, mediaType, label, path }) { * folds away on its own. * * A pull request page passes `comments`, this file's anchored statements - * keyed the way `lineKey` keys a row, and `onComment` to publish a new one; + * keyed the way `lineKey` keys a row, `spans` the ranges a comment covers, + * and `onComment` to publish a new one; * each renders under the line it speaks to, which is what an anchor is for. A * plain commit page passes neither and reads as it always did. */ -export function DiffFile({ file, ref, comments, onComment }) { +export function DiffFile({ file, ref, comments, spans, onComment }) { const language = languageFor(file.path); const [open, setOpen] = useState(true); // Colouring a whole file twice costs more than colouring a hunk, so it @@ -85,16 +86,27 @@ export function DiffFile({ file, ref, comments, onComment }) { // whole change, and the status says which side it is. const twoImages = Boolean(file.oldImage && file.newImage); - const post = async (line) => { + const post = async () => { const target = composing; setBusy(true); setError(''); try { + const range = target?.range; await onComment({ path: file.path, - line: line.newLine ?? line.oldLine, - side: line.newLine != null ? 'new' : 'old', - snippet: line.text.slice(0, 512), + ...(range + ? { + line: range.start, + side: range.side, + snippet: range.startText.slice(0, 512), + ...(range.end !== range.start + ? { + endLine: range.end, + endSnippet: range.endText.slice(0, 512), + } + : {}), + } + : {}), replyTo: target?.replyTo, note: note.trim(), }); @@ -109,16 +121,82 @@ export function DiffFile({ file, ref, comments, onComment }) { } }; - const compose = (key, replyTo) => { - setComposing( - composing?.key === key && composing?.replyTo?.uri === replyTo?.uri - ? null - : { key, ...(replyTo ? { replyTo } : {}) }, - ); + /** + * Open the composer on a line, extend a range to it with shift, or answer a + * comment. A shift-click on the gutter of a line, with a fresh comment + * already open on the same side, makes the two ends of a range; the thread + * then speaks to every line between them. + * @param {string} key @param {Object} line + * @param {{uri: string, cid: string}} [replyTo] + * @param {boolean} [shift] + */ + const compose = (key, line, replyTo, shift) => { + if (replyTo) { + setComposing((cur) => + cur?.key === key && cur?.replyTo?.uri === replyTo.uri + ? null + : { key, replyTo }, + ); + setNote(''); + setError(''); + return; + } + const side = line.newLine != null ? 'new' : 'old'; + const at = line.newLine ?? line.oldLine; + setComposing((cur) => { + if (shift && cur?.range && cur.range.side === side) { + const anchor = cur.range.anchor; + const [start, end] = anchor <= at ? [anchor, at] : [at, anchor]; + const [startText, endText] = + anchor <= at + ? [cur.range.anchorText, line.text] + : [line.text, cur.range.anchorText]; + return { + key, + range: { + side, + start, + end, + startText, + endText, + anchor, + anchorText: cur.range.anchorText, + }, + }; + } + if (cur?.key === key && !cur.replyTo) return null; + return { + key, + range: { + side, + start: at, + end: at, + startText: line.text, + endText: line.text, + anchor: at, + anchorText: line.text, + }, + }; + }); setNote(''); setError(''); }; + /** + * Whether a row is in the range the composer is selecting now, so it marks + * the span a reader is about to comment on before they publish it. + * @param {Object} line + */ + const selecting = (line) => { + const range = composing?.range; + if (!range) return false; + const side = line.newLine != null ? 'new' : 'old'; + const at = line.newLine ?? line.oldLine; + return ( + side === range.side && at != null && at >= range.start && at <= range.end + ); + }; + // The gutter is cut to the widest line number this file will show. A fixed // width fits whatever it was measured against and clips everything longer, // and a file of a few thousand lines is ordinary. @@ -208,6 +286,8 @@ export function DiffFile({ file, ref, comments, onComment }) { className={cn( 'group relative flex w-full whitespace-pre', ROW_STYLE[line.type], + (selecting(line) || inSpan(spans, line)) && + 'bg-key/10', )} > {/* Two columns of numbers cost a quarter of a phone's @@ -239,8 +319,10 @@ export function DiffFile({ file, ref, comments, onComment }) { {onComment && ( diff --git a/packages/git-ui/src/lib/comments.js b/packages/git-ui/src/lib/comments.js index 1977190..854a8ca 100644 --- a/packages/git-ui/src/lib/comments.js +++ b/packages/git-ui/src/lib/comments.js @@ -8,11 +8,14 @@ // would search for; nothing here pretends to that yet. /** - * @param {{line?: number, side?: string}} anchor - * @returns {string} the key a diff row computes for itself + * The key a diff row computes for a comment that sits on it. A range sits on + * its last line, where a forge hangs a multi-line thread, so the row that + * carries the conversation is the end of the span. + * @param {{line?: number, endLine?: number, side?: string}} anchor + * @returns {string} */ export function anchorKey(anchor) { - return `${anchor.side === 'old' ? 'old' : 'new'}:${anchor.line ?? 0}`; + return `${anchor.side === 'old' ? 'old' : 'new'}:${anchor.endLine ?? anchor.line ?? 0}`; } /** @@ -129,12 +132,16 @@ export function threadSummaries(statements) { * @param {{sha: string, author?: string|null, ref?: string|null}} version - * the version in view, and the pull request it belongs to * @returns {{byFile: Map>>>, - * elsewhere: number}} comments per file and line, and how many are on other - * versions of the same pull request + * spans: Map>, + * elsewhere: number}} comments per file and line, the ranges a comment + * covers per file for the diff to mark, and how many are on other versions + * of the same pull request */ export function placeComments(statements, version) { /** @type {Map>>>} */ const byFile = new Map(); + /** @type {Map>} */ + const spans = new Map(); let elsewhere = 0; for (const statement of statements) { const anchor = /** @type {{path?: unknown}} */ (statement.anchor ?? {}); @@ -153,11 +160,39 @@ export function placeComments(statements, version) { const key = anchorKey(/** @type {*} */ (anchor)); lines.set(key, [...(lines.get(key) ?? []), statement]); byFile.set(anchor.path, lines); + const range = + /** @type {{line?: number, endLine?: number, side?: string}} */ (anchor); + if (typeof range.endLine === 'number' && typeof range.line === 'number') { + const list = spans.get(anchor.path) ?? []; + list.push({ + side: range.side === 'old' ? 'old' : 'new', + start: Math.min(range.line, range.endLine), + end: Math.max(range.line, range.endLine), + }); + spans.set(anchor.path, list); + } } for (const lines of byFile.values()) { for (const [key, stack] of lines) { lines.set(key, threadStack(stack)); } } - return { byFile, elsewhere }; + return { byFile, spans, elsewhere }; +} + +/** + * Whether a diff row falls inside a range a comment covers, so the diff can + * mark the span its last line carries the thread for. + * @param {Array<{side: string, start: number, end: number}>|undefined} spans + * @param {{oldLine: number|null, newLine: number|null}} line + * @returns {boolean} + */ +export function inSpan(spans, line) { + if (!spans) return false; + const side = line.newLine != null ? 'new' : 'old'; + const at = line.newLine ?? line.oldLine; + if (at == null) return false; + return spans.some( + (span) => span.side === side && at >= span.start && at <= span.end, + ); } diff --git a/packages/git-ui/src/lib/publish.js b/packages/git-ui/src/lib/publish.js index 62b64b0..39b4c2f 100644 --- a/packages/git-ui/src/lib/publish.js +++ b/packages/git-ui/src/lib/publish.js @@ -23,8 +23,9 @@ const ISSUE_COLLECTION = 'dev.pdsjs.git.issue'; * what identifies the pull request * @param {string} about.verdict - approve, changesRequested or comment * @param {string} about.note - * @param {{path: string, line?: number, side?: string, snippet?: string}} [about.anchor] - - * the line spoken to, for a comment on one rather than on the whole + * @param {{path: string, line?: number, endLine?: number, side?: string, snippet?: string, endSnippet?: string}} [about.anchor] - + * the line, or the range, spoken to, for a comment on the code rather than + * on the whole * @param {{uri: string, cid: string}} [about.replyTo] - the review answered, * by its exact version * @returns {Record} diff --git a/packages/git-ui/src/pages/commit.jsx b/packages/git-ui/src/pages/commit.jsx index dd8291f..c19a872 100644 --- a/packages/git-ui/src/pages/commit.jsx +++ b/packages/git-ui/src/pages/commit.jsx @@ -437,6 +437,7 @@ export function CommitPage({ key={file.path} file={file} comments={comments.byFile.get(file.path)} + spans={comments.spans.get(file.path)} onComment={reviewable && session ? onComment : undefined} ref={(node) => { if (node) sections.current.set(file.path, node); diff --git a/packages/git-ui/test/collab.test.js b/packages/git-ui/test/collab.test.js index 39db1ec..0e2afb6 100644 --- a/packages/git-ui/test/collab.test.js +++ b/packages/git-ui/test/collab.test.js @@ -9,6 +9,7 @@ import { describe, expect, it } from 'vitest'; import { diffHref, parsePullPath } from '../src/lib/collab.js'; import { anchorKey, + inSpan, lineKey, placeComments, threadStack, @@ -203,6 +204,42 @@ describe('threadSummaries', () => { }); }); +describe('placeComments, on a range', () => { + const version = { sha: sha('t'), author: null, ref: null }; + const review = (n, anchor) => ({ + uri: `at://x/dev.pdsjs.git.review/${n}`, + sha: sha('t'), + verdict: 'comment', + reviewedAt: `2026-08-0${n}T00:00:00Z`, + anchor, + by: { did: 'did:plc:a' }, + }); + + it('hangs a range thread on its last line and marks the span between', () => { + const { byFile, spans } = placeComments( + [ + review(1, { path: 'a.js', line: 3, endLine: 6, side: 'new' }), + review(2, { path: 'a.js', line: 9, side: 'new' }), + ], + version, + ); + const lines = byFile.get('a.js'); + // The range sits on line 6, the single comment on line 9. + expect([...lines.keys()].sort()).toEqual(['new:6', 'new:9']); + expect(spans.get('a.js')).toEqual([{ side: 'new', start: 3, end: 6 }]); + }); + + it('reads a row as inside a span on its own side only', () => { + const spans = [{ side: 'new', start: 3, end: 6 }]; + expect(inSpan(spans, { oldLine: null, newLine: 4 })).toBe(true); + expect(inSpan(spans, { oldLine: null, newLine: 6 })).toBe(true); + expect(inSpan(spans, { oldLine: null, newLine: 7 })).toBe(false); + // Same number, other side: not in the span. + expect(inSpan(spans, { oldLine: 4, newLine: null })).toBe(false); + expect(inSpan(undefined, { oldLine: null, newLine: 4 })).toBe(false); + }); +}); + describe('placeComments', () => { const version = { sha: sha('a'), diff --git a/packages/git/src/lexicon.js b/packages/git/src/lexicon.js index ef5a287..d8efbf1 100644 --- a/packages/git/src/lexicon.js +++ b/packages/git/src/lexicon.js @@ -351,19 +351,31 @@ export const gitReviewLexicon = { type: 'integer', minimum: 1, description: - 'One-based, in the file the sha names, so placement on that version is exact whatever happens to the branch.', + 'One-based, in the file the sha names, so placement on that version is exact whatever happens to the branch. The first line of a range, or the only line.', + }, + endLine: { + type: 'integer', + minimum: 1, + description: + 'The last line of a range, where the comment speaks to several. Absent for a comment on one line. A reader shows the thread at this line and marks the span from line to it.', }, side: { type: 'string', knownValues: ['old', 'new'], description: - "Which side of the change's diff counts the line; new when absent.", + "Which side of the change's diff counts the lines; new when absent. A range is on one side.", }, snippet: { type: 'string', maxLength: 512, description: - 'The line itself. A rebase moves numbers; the words are what a later version searches for to keep the comment placed.', + 'The first line itself. A rebase moves numbers; the words are what a later version searches for to keep the comment placed.', + }, + endSnippet: { + type: 'string', + maxLength: 512, + description: + 'The last line of a range, for the same reason snippet keeps the first.', }, }, },