diff --git a/src/components/ReviewPage.svelte b/src/components/ReviewPage.svelte index a18467db7..4ffafb816 100644 --- a/src/components/ReviewPage.svelte +++ b/src/components/ReviewPage.svelte @@ -39,13 +39,18 @@ import { anchorOf } from "@app/lib/pierreComments"; import * as roles from "@app/lib/roles"; import { push } from "@app/lib/router"; + import { + buildThreads, + groupThreadsByFile, + isCodeRoot, + reviewDiscussionThreads, + } from "@app/lib/threads"; import { absoluteTimestamp, authorForNodeId, didFromPublicKey, formatTimestamp, pluralize, - publicKeyFromDid, revisionPosition, verdictAction, verdictBadge, @@ -247,20 +252,9 @@ } } - // Comments on the review itself rather than on a line. The summary is a - // field, not the root of this thread, so a comment replying to the review id - // starts a root thread of its own. - const commentThreads: Thread[] = $derived.by(() => { - const comments = (review.comments ?? []) as Comment[]; - return comments - .filter(c => (!c.location && !c.replyTo) || c.replyTo === review.id) - .map(root => ({ - root, - replies: comments - .filter(c => c.replyTo === root.id) - .sort((a, b) => a.edits[0].timestamp - b.edits[0].timestamp), - })) as unknown as Thread[]; - }); + const commentThreads = $derived( + reviewDiscussionThreads(review) as unknown as Thread[], + ); async function createDiscussionComment( body: string, @@ -328,21 +322,15 @@ // revision comments have no resolve action at all. function canResolveComment(commentId: string): boolean { if (ownerOf(commentId)?.kind !== "review") return false; - if ( - roles.isDelegate( - config.publicKey, - repoDelegates.map(d => d.did), - ) - ) { - return true; - } - const comment = (review.comments ?? []).find(c => c.id === commentId); - return [ - comment?.author.did, - review.author.did, - reviewedRevision?.author.did, - ].some( - did => did !== undefined && publicKeyFromDid(did) === config.publicKey, + return roles.canResolveReviewComment( + config.publicKey, + repoDelegates.map(d => d.did), + { + comment: (review.comments ?? []).find(c => c.id === commentId)?.author + .did, + review: review.author.did, + revision: reviewedRevision?.author.did, + }, ); } @@ -357,35 +345,22 @@ }), ); - const fileGroups: FileGroup[] = $derived.by(() => { - // eslint-disable-next-line svelte/prefer-svelte-reactivity - const groups = new Map[]>(); - const addThreads = ( - comments: Comment[], - excludeId: string, - ) => { - const roots = comments.filter( - c => c.location && !c.replyTo && c.id !== excludeId, - ); - for (const root of roots) { - const replies = comments - .filter(c => c.replyTo === root.id) - .sort((a, b) => a.edits[0].timestamp - b.edits[0].timestamp); - const thread = { root, replies } as Thread; - const path = root.location!.path; - const list = groups.get(path) ?? []; - list.push(thread); - groups.set(path, list); - } - }; - addThreads(review.comments as Comment[], review.id); - // Standalone code comments left directly on the reviewed revision (not part - // of the review itself) also belong on this page. - if (reviewedRevision?.discussion) { - addThreads(reviewedRevision.discussion, reviewedRevision.id); - } - return [...groups.entries()].map(([path, threads]) => ({ path, threads })); - }); + // Standalone code comments left directly on the reviewed revision (not part + // of the review itself) also belong on this page. + const fileGroups: FileGroup[] = $derived( + groupThreadsByFile([ + ...buildThreads( + review.comments as Comment[], + c => isCodeRoot(c) && c.id !== review.id, + ), + ...(reviewedRevision + ? buildThreads( + reviewedRevision.discussion ?? [], + c => isCodeRoot(c) && c.id !== reviewedRevision.id, + ) + : []), + ]), + ); // Standalone comments render next to the review's own and look identical, // but nothing can resolve them and their edits target the revision, so they diff --git a/src/components/Revision.svelte b/src/components/Revision.svelte index b944a49d6..5c12ab88d 100644 --- a/src/components/Revision.svelte +++ b/src/components/Revision.svelte @@ -34,6 +34,16 @@ import { patchTimeline } from "@app/lib/patchTimeline"; import * as roles from "@app/lib/roles"; import { push } from "@app/lib/router"; + import { + buildThreads, + codeThreadsByReview, + discussionThreadsByReview as discussionThreadsByReviewOf, + groupThreadsByFile, + isCodeRoot, + reviewIdByComment, + revisionDiscussionThreads, + revisionIdByComment, + } from "@app/lib/threads"; import { authorForNodeId, didFromPublicKey, @@ -255,14 +265,7 @@ const hasPublishedReview = $derived(Boolean(ownPublishedReview)); const codeCommentThreads: Thread[] = $derived( - draftReview - ? (draftReview.comments - .filter(c => c.location && !c.replyTo) - .map(root => ({ - root, - replies: draftReview.comments.filter(c => c.replyTo === root.id), - })) as Thread[]) - : [], + draftReview ? buildThreads(draftReview.comments, isCodeRoot) : [], ); async function createCodeComment( @@ -379,72 +382,18 @@ } } - // One header per file with its hunks stacked beneath, rather than a - // duplicate file block per comment. - function groupThreadsByFile( - list: Thread[], - ): { path: string; threads: Thread[] }[] { - // eslint-disable-next-line svelte/prefer-svelte-reactivity - const groups = new Map[]>(); - for (const thread of list) { - const path = thread.root.location?.path; - if (!path) continue; - const existing = groups.get(path) ?? []; - existing.push(thread); - groups.set(path, existing); - } - return [...groups.entries()].map(([path, threads]) => ({ path, threads })); - } - // Comments on a review itself rather than on a line. Kept apart from // `threadsByReview`, which feeds the diff and must only carry threads that // can be anchored to a line. - const discussionThreadsByReview = $derived.by(() => { - // eslint-disable-next-line svelte/prefer-svelte-reactivity - const map = new Map[]>(); - (revision.reviews ?? []).forEach(review => { - const comments = review.comments ?? []; - const threads = comments - .filter(c => (!c.location && !c.replyTo) || c.replyTo === review.id) - .map(root => ({ - root, - replies: comments - .filter(c => c.replyTo === root.id) - .sort((a, b) => a.edits[0].timestamp - b.edits[0].timestamp), - })) as Thread[]; - if (threads.length > 0) { - map.set(review.id, threads); - } - }); - return map; - }); + const discussionThreadsByReview = $derived( + discussionThreadsByReviewOf(revision.reviews ?? []), + ); - const commentToReviewId = $derived.by(() => { - // eslint-disable-next-line svelte/prefer-svelte-reactivity - const map = new Map(); - for (const source of [threadsByReview, discussionThreadsByReview]) { - for (const [reviewId, threads] of source.entries()) { - for (const thread of threads) { - map.set(thread.root.id, reviewId); - for (const reply of thread.replies) { - map.set(reply.id, reviewId); - } - } - } - } - return map; - }); + const commentToReviewId = $derived.by(() => + reviewIdByComment(threadsByReview, discussionThreadsByReview), + ); - const commentToRevisionId = $derived.by(() => { - // eslint-disable-next-line svelte/prefer-svelte-reactivity - const map = new Map(); - for (const rev of revisions) { - for (const comment of rev.discussion ?? []) { - map.set(comment.id, rev.id); - } - } - return map; - }); + const commentToRevisionId = $derived(revisionIdByComment(revisions)); // Every code-comment mutation dispatches on the owning object; resolving it // once here keeps the actions shared. @@ -473,18 +422,15 @@ function canResolveComment(commentId: string): boolean { const owner = ownerOf(commentId); if (owner?.kind !== "review") return false; - if ( - roles.isDelegate( - config.publicKey, - repoDelegates.map(d => d.did), - ) - ) { - return true; - } const review = (revision.reviews ?? []).find(r => r.id === owner.reviewId); - const comment = review?.comments?.find(c => c.id === commentId); - return [comment?.author.did, review?.author.did, revision.author.did].some( - did => did !== undefined && publicKeyFromDid(did) === config.publicKey, + return roles.canResolveReviewComment( + config.publicKey, + repoDelegates.map(d => d.did), + { + comment: review?.comments?.find(c => c.id === commentId)?.author.did, + review: review?.author.did, + revision: revision.author.did, + }, ); } @@ -538,15 +484,7 @@ // render on the diff alongside review comments instead of vanishing. const revisionCodeCommentThreads: Thread[] = $derived.by(() => { if (hiddenCommentSources.includes(STANDALONE_COMMENTS)) return []; - const discussion = revision.discussion ?? []; - return discussion - .filter(c => c.location && !c.replyTo) - .map(root => ({ - root, - replies: discussion - .filter(c => c.replyTo === root.id) - .sort((a, b) => a.edits[0].timestamp - b.edits[0].timestamp), - })) as Thread[]; + return buildThreads(revision.discussion ?? [], isCodeRoot); }); const codeComments: CodeComments | undefined = $derived.by(() => { @@ -643,25 +581,7 @@ new Set(revisions.filter(r => r.id !== latestRevisionId).map(r => r.id)), ); - const threadsByReview = $derived.by(() => { - // eslint-disable-next-line svelte/prefer-svelte-reactivity - const map = new Map[]>(); - (revision.reviews ?? []).forEach(review => { - const reviewComments = review.comments ?? []; - const threads = reviewComments - .filter(c => c.location && !c.replyTo) - .map(root => { - const replies = reviewComments - .filter(c => c.replyTo === root.id) - .sort((a, b) => a.edits[0].timestamp - b.edits[0].timestamp); - return { root, replies } as Thread; - }); - if (threads.length > 0) { - map.set(review.id, threads); - } - }); - return map; - }); + const threadsByReview = $derived(codeThreadsByReview(revision.reviews ?? [])); const activityItems = $derived( patchTimeline({ @@ -673,45 +593,11 @@ expandedRevisionRuns, }), ); - const reviewSummaryFingerprints = $derived( - new Set( - revisions - .flatMap(r => r.reviews ?? []) - .filter(r => r.summary && r.summary.trim() !== "") - .map(r => `${r.author.did} ${r.summary}`), - ), - ); // Gather discussion comments from every revision, not just the selected one, // so comments left on a revision stay in the timeline after a newer revision // is pushed. Discussion.svelte orders the merged timeline by timestamp. const commentThreads = $derived( - revisions.flatMap(rev => { - const discussion = rev.discussion; - if (!discussion) return []; - return ( - discussion - .filter( - comment => - (comment.id !== rev.id && !comment.replyTo) || - comment.replyTo === rev.id, - ) - // Code comments (those with a location) render on the diff, not as - // plain entries in the activity timeline. - .filter(comment => !comment.location) - .filter(comment => { - const body = comment.edits[comment.edits.length - 1]?.body ?? ""; - return !reviewSummaryFingerprints.has( - `${comment.author.did} ${body}`, - ); - }) - .map(thread => ({ - root: thread, - replies: discussion - .filter(comment => comment.replyTo === thread.id) - .sort((a, b) => a.edits[0].timestamp - b.edits[0].timestamp), - })) - ); - }) as Thread[], + revisionDiscussionThreads(revisions) as unknown as Thread[], ); async function editRevision( diff --git a/src/lib/roles.ts b/src/lib/roles.ts index 81b6bac9c..12d05ba22 100644 --- a/src/lib/roles.ts +++ b/src/lib/roles.ts @@ -24,3 +24,20 @@ export function isDelegateOrAuthor( undefined ); } + +/// The protocol lets the comment author, the review author or the revision +/// author resolve a review comment; delegates may do anything. +export function canResolveReviewComment( + publicKey: string, + delegates: string[], + authors: { + comment: string | undefined; + review: string | undefined; + revision: string | undefined; + }, +): boolean { + if (isDelegate(publicKey, delegates)) return true; + return [authors.comment, authors.review, authors.revision].some( + did => did !== undefined && publicKeyFromDid(did) === publicKey, + ); +} diff --git a/src/lib/threads.ts b/src/lib/threads.ts new file mode 100644 index 000000000..6368e02e5 --- /dev/null +++ b/src/lib/threads.ts @@ -0,0 +1,132 @@ +import type { Review } from "@bindings/cob/patch/Review"; +import type { Revision } from "@bindings/cob/patch/Revision"; +import type { CodeLocation } from "@bindings/cob/thread/CodeLocation"; +import type { Comment } from "@bindings/cob/thread/Comment"; +import type { Thread } from "@bindings/cob/thread/Thread"; + +/// Threads rooted at the comments `isRoot` picks, each with its replies oldest +/// first. +export function buildThreads( + comments: Comment[], + isRoot: (comment: Comment) => boolean, +): Thread[] { + return comments.filter(isRoot).map(root => ({ + root, + replies: comments + .filter(c => c.replyTo === root.id) + .sort((a, b) => a.edits[0].timestamp - b.edits[0].timestamp), + })); +} + +/// A code comment that starts a thread. +export function isCodeRoot(comment: Comment): boolean { + return Boolean(comment.location) && !comment.replyTo; +} + +/// Each review's code threads, for reviews that have any. +export function codeThreadsByReview( + reviews: Review[], +): Map[]> { + const map = new Map[]>(); + for (const review of reviews) { + const threads = buildThreads(review.comments ?? [], isCodeRoot); + if (threads.length > 0) map.set(review.id, threads); + } + return map; +} + +/// Comments on a review itself rather than on a line. The summary is a field, +/// not the root of this thread, so a comment replying to the review id starts +/// a thread of its own. +export function reviewDiscussionThreads( + review: Review, +): Thread[] { + return buildThreads( + review.comments ?? [], + c => (!c.location && !c.replyTo) || c.replyTo === review.id, + ); +} + +/// Each review's discussion threads, for reviews that have any. +export function discussionThreadsByReview( + reviews: Review[], +): Map[]> { + const map = new Map[]>(); + for (const review of reviews) { + const threads = reviewDiscussionThreads(review); + if (threads.length > 0) map.set(review.id, threads); + } + return map; +} + +/// Which review each comment in these threads belongs to. +export function reviewIdByComment( + ...sources: Map[]>[] +): Map { + const map = new Map(); + for (const source of sources) { + for (const [reviewId, threads] of source) { + for (const thread of threads) { + map.set(thread.root.id, reviewId); + for (const reply of thread.replies) map.set(reply.id, reviewId); + } + } + } + return map; +} + +/// Which revision each discussion comment was left on. +export function revisionIdByComment( + revisions: Revision[], +): Map { + const map = new Map(); + for (const revision of revisions) { + for (const comment of revision.discussion ?? []) { + map.set(comment.id, revision.id); + } + } + return map; +} + +/// One group per file, in order of first appearance, so a file's threads +/// render under a single header. +export function groupThreadsByFile( + threads: Thread[], +): { path: string; threads: Thread[] }[] { + const groups = new Map[]>(); + for (const thread of threads) { + const path = thread.root.location?.path; + if (!path) continue; + const group = groups.get(path) ?? []; + group.push(thread); + groups.set(path, group); + } + return [...groups.entries()].map(([path, threads]) => ({ path, threads })); +} + +/// The discussion threads of every revision, for the activity timeline. +/// +/// Code comments render on the diff instead. A comment whose body repeats one +/// of its author's review summaries is left out too, since the review already +/// shows that text. +export function revisionDiscussionThreads( + revisions: Revision[], +): Thread[] { + const summaries = new Set( + revisions + .flatMap(r => r.reviews ?? []) + .filter(r => r.summary && r.summary.trim() !== "") + .map(r => `${r.author.did} ${r.summary}`), + ); + return revisions.flatMap(revision => + buildThreads(revision.discussion ?? [], comment => { + const body = comment.edits[comment.edits.length - 1]?.body ?? ""; + return ( + ((comment.id !== revision.id && !comment.replyTo) || + comment.replyTo === revision.id) && + !comment.location && + !summaries.has(`${comment.author.did} ${body}`) + ); + }), + ); +} diff --git a/tests/unit/roles.test.ts b/tests/unit/roles.test.ts new file mode 100644 index 000000000..b870f0daa --- /dev/null +++ b/tests/unit/roles.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, test } from "vitest"; + +import { + canResolveReviewComment, + isDelegate, + isDelegateOrAuthor, +} from "@app/lib/roles"; + +const me = "z6Mkme"; +const delegates = ["did:key:z6Mkdelegate", "did:key:z6Mkother"]; + +describe("isDelegate", () => { + test("matches a delegate's public key", () => { + expect(isDelegate("z6Mkdelegate", delegates)).toBe(true); + }); + + test.each([[me], [undefined]])("is undefined for %j", key => { + expect(isDelegate(key, delegates)).toBeUndefined(); + }); +}); + +describe("isDelegateOrAuthor", () => { + test("allows delegates and the author", () => { + expect(isDelegateOrAuthor("z6Mkdelegate", delegates, "did:key:x")).toBe( + true, + ); + expect(isDelegateOrAuthor(me, delegates, `did:key:${me}`)).toBe(true); + }); + + test("is undefined for anyone else", () => { + expect(isDelegateOrAuthor(me, delegates, "did:key:x")).toBeUndefined(); + expect( + isDelegateOrAuthor(undefined, delegates, "did:key:x"), + ).toBeUndefined(); + }); +}); + +describe("canResolveReviewComment", () => { + const nobody = { comment: undefined, review: undefined, revision: undefined }; + + test("a delegate can resolve any review comment", () => { + expect(canResolveReviewComment("z6Mkdelegate", delegates, nobody)).toBe( + true, + ); + }); + + test.each(["comment", "review", "revision"] as const)( + "the %s author can resolve it", + role => { + expect( + canResolveReviewComment(me, delegates, { + ...nobody, + [role]: `did:key:${me}`, + }), + ).toBe(true); + }, + ); + + test("nobody else can", () => { + expect( + canResolveReviewComment(me, delegates, { + comment: "did:key:a", + review: "did:key:b", + revision: "did:key:c", + }), + ).toBe(false); + }); +}); diff --git a/tests/unit/threads.test.ts b/tests/unit/threads.test.ts new file mode 100644 index 000000000..d305ed8d3 --- /dev/null +++ b/tests/unit/threads.test.ts @@ -0,0 +1,181 @@ +import { describe, expect, test } from "vitest"; + +import { + buildThreads, + codeThreadsByReview, + discussionThreadsByReview, + groupThreadsByFile, + isCodeRoot, + reviewDiscussionThreads, + reviewIdByComment, + revisionDiscussionThreads, + revisionIdByComment, +} from "@app/lib/threads"; + +import { + author, + comment, + location, + review, + revision, + thread, +} from "./support/cobs"; + +const alice = author("alice"); +const bob = author("bob"); +const code = location("a.ts", "new", 1); + +function ids(threads: { root: { id: string }; replies: { id: string }[] }[]) { + return threads.map(t => [t.root.id, ...t.replies.map(r => r.id)]); +} + +describe("buildThreads", () => { + test("attaches replies to their root, oldest first", () => { + const threads = buildThreads( + [ + comment({ id: "root" }), + comment({ id: "late", replyTo: "root", timestamp: 20 }), + comment({ id: "early", replyTo: "root", timestamp: 10 }), + comment({ id: "other" }), + ], + c => !c.replyTo, + ); + + expect(ids(threads)).toEqual([["root", "early", "late"], ["other"]]); + }); + + test("isCodeRoot picks located comments that aren't replies", () => { + expect(isCodeRoot(comment({ location: code }))).toBe(true); + expect(isCodeRoot(comment({ location: code, replyTo: "x" }))).toBe(false); + expect(isCodeRoot(comment({ location: null }))).toBe(false); + }); +}); + +describe("threads by review", () => { + const reviews = [ + review({ + id: "r1", + comments: [ + comment({ id: "code", location: code }), + comment({ id: "code-reply", location: code, replyTo: "code" }), + comment({ id: "general" }), + comment({ id: "on-review", location: code, replyTo: "r1" }), + ], + }), + review({ id: "r2", comments: [] }), + ]; + + test("codeThreadsByReview keeps reviews with code threads", () => { + const map = codeThreadsByReview(reviews); + + expect([...map.keys()]).toEqual(["r1"]); + expect(ids(map.get("r1") ?? [])).toEqual([["code", "code-reply"]]); + }); + + test("a reply to the review itself starts a discussion thread", () => { + expect(ids(reviewDiscussionThreads(reviews[0]))).toEqual([ + ["general"], + ["on-review"], + ]); + }); + + test("discussionThreadsByReview keeps reviews with discussion threads", () => { + expect([...discussionThreadsByReview(reviews).keys()]).toEqual(["r1"]); + }); + + test("reviewIdByComment maps roots and replies to their review", () => { + const map = reviewIdByComment( + codeThreadsByReview(reviews), + discussionThreadsByReview(reviews), + ); + + expect(Object.fromEntries(map)).toEqual({ + code: "r1", + "code-reply": "r1", + general: "r1", + "on-review": "r1", + }); + }); +}); + +test("revisionIdByComment maps discussion comments to their revision", () => { + const map = revisionIdByComment([ + revision({ id: "v1", discussion: [comment({ id: "a" })] }), + revision({ id: "v2", discussion: [comment({ id: "b" })] }), + ]); + + expect(Object.fromEntries(map)).toEqual({ a: "v1", b: "v2" }); +}); + +test("groupThreadsByFile groups by path in order of first appearance", () => { + const groups = groupThreadsByFile([ + thread(comment({ id: "b1", location: location("b.ts", "new", 1) })), + thread(comment({ id: "a1", location: location("a.ts", "new", 1) })), + thread(comment({ id: "b2", location: location("b.ts", "old", 4) })), + thread(comment({ id: "none", location: null })), + ]); + + expect(groups.map(g => [g.path, ...g.threads.map(t => t.root.id)])).toEqual([ + ["b.ts", "b1", "b2"], + ["a.ts", "a1"], + ]); +}); + +describe("revisionDiscussionThreads", () => { + test("collects each revision's plain comments and replies to it", () => { + const threads = revisionDiscussionThreads([ + revision({ + id: "v1", + discussion: [ + comment({ id: "c1" }), + comment({ id: "reply", replyTo: "c1" }), + comment({ id: "code", location: code }), + comment({ id: "on-revision", replyTo: "v1" }), + ], + }), + revision({ id: "v2", discussion: [comment({ id: "c2" })] }), + ]); + + expect(ids(threads)).toEqual([["c1", "reply"], ["on-revision"], ["c2"]]); + }); + + test("skips the revision's own description comment", () => { + expect( + revisionDiscussionThreads([ + revision({ id: "v1", discussion: [comment({ id: "v1" })] }), + ]), + ).toEqual([]); + }); + + test("skips a comment repeating its author's review summary", () => { + const threads = revisionDiscussionThreads([ + revision({ + id: "v1", + reviews: [review({ author: alice, summary: "LGTM" })], + discussion: [ + comment({ + id: "repeat", + author: alice, + edits: [edit(alice, "LGTM")], + }), + comment({ + id: "other-author", + author: bob, + edits: [edit(bob, "LGTM")], + }), + comment({ + id: "edited", + author: alice, + edits: [edit(alice, "LGTM"), edit(alice, "Changed")], + }), + ], + }), + ]); + + expect(ids(threads)).toEqual([["other-author"], ["edited"]]); + }); +}); + +function edit(by: typeof alice, body: string) { + return { author: by, timestamp: 0, body }; +}