diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts index 0e62a3a615adc..3283e647fcf38 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts @@ -1012,93 +1012,127 @@ layer("GitLabPullRequestCli.layer", (it) => { }), ); - it.effect("reads a positioned discussion as a thread anchored to its line", () => - Effect.gen(function* () { - mockedExecute.mockReturnValueOnce( - Effect.succeed( - output( - JSON.stringify([ - { - id: "abc123", - notes: [ - { - id: 1, - body: "rename this", - author: { username: "bilal", avatar_url: "https://avatars/b.png" }, - created_at: "2026-07-01T00:00:00Z", - resolvable: true, - resolved: true, - position: { - position_type: "text", - new_path: "src/a.ts", - old_path: "src/a.ts", - new_line: 12, - old_line: null, + it.effect( + "reads a positioned discussion as a thread and an open overview discussion as a finding", + () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + JSON.stringify([ + { + id: "abc123", + notes: [ + { + id: 1, + body: "rename this", + author: { username: "bilal", avatar_url: "https://avatars/b.png" }, + created_at: "2026-07-01T00:00:00Z", + resolvable: true, + resolved: true, + position: { + position_type: "text", + new_path: "src/a.ts", + old_path: "src/a.ts", + new_line: 12, + old_line: null, + }, }, - }, - { - id: 2, - body: "done", - author: { username: "julius" }, - created_at: "2026-07-01T01:00:00Z", - }, - ], - }, - { id: "def456", notes: [{ id: 3, body: "ship it", created_at: "2026-07-01Z" }] }, - { - id: "file789", - notes: [ - { - id: 4, - body: "split this file", - created_at: "2026-07-01Z", - position: { - position_type: "file", - new_path: "src/b.ts", - old_path: "src/b.ts", - new_line: null, - old_line: null, + { + id: 2, + body: "done", + author: { username: "julius" }, + created_at: "2026-07-01T01:00:00Z", }, - }, - ], - }, - { - id: "img012", - notes: [ - { - id: 5, - body: "logo is blurry", - created_at: "2026-07-01Z", - position: { - position_type: "image", - new_path: "logo.png", - old_path: "logo.png", - new_line: null, - old_line: null, + ], + }, + { id: "def456", notes: [{ id: 3, body: "ship it", created_at: "2026-07-01Z" }] }, + { + id: "ghi789", + notes: [ + { + id: 4, + body: "mention the new flag in the README", + created_at: "2026-07-01T02:00:00Z", + resolvable: true, + resolved: false, }, - }, - ], - }, - ]), + { + id: 5, + body: "will do", + created_at: "2026-07-01T03:00:00Z", + resolvable: true, + resolved: false, + }, + ], + }, + { + id: "jkl012", + notes: [ + { + id: 6, + body: "already addressed", + created_at: "2026-07-01T04:00:00Z", + resolvable: true, + resolved: true, + }, + ], + }, + { + id: "file789", + notes: [ + { + id: 7, + body: "split this file", + created_at: "2026-07-01Z", + position: { + position_type: "file", + new_path: "src/b.ts", + old_path: "src/b.ts", + new_line: null, + old_line: null, + }, + }, + ], + }, + { + id: "img012", + notes: [ + { + id: 8, + body: "logo is blurry", + created_at: "2026-07-01Z", + position: { + position_type: "image", + new_path: "logo.png", + old_path: "logo.png", + new_line: null, + old_line: null, + }, + }, + ], + }, + ]), + ), ), - ), - ); - const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + ); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; - const { threads } = yield* cli.listDiscussions({ - cwd: "/w", - repository: "acme/web", - number: 7, - }); + const { threads, overviewFindingIds } = yield* cli.listDiscussions({ + cwd: "/w", + repository: "acme/web", + number: 7, + }); - expect(threads.map(({ id, path, line, side }) => ({ id, path, line, side }))).toEqual([ - { id: "abc123", path: "src/a.ts", line: 12, side: "right" }, - { id: "file789", path: "src/b.ts", line: null, side: "right" }, - { id: "img012", path: "logo.png", line: null, side: "right" }, - ]); - expect(threads[0]).toMatchObject({ isResolved: true }); - assert.strictEqual(threads[0]?.comments.length, 2); - }), + expect(overviewFindingIds).toEqual(["4"]); + expect(threads.map(({ id, path, line, side }) => ({ id, path, line, side }))).toEqual([ + { id: "abc123", path: "src/a.ts", line: 12, side: "right" }, + { id: "file789", path: "src/b.ts", line: null, side: "right" }, + { id: "img012", path: "logo.png", line: null, side: "right" }, + ]); + expect(threads[0]).toMatchObject({ isResolved: true }); + assert.strictEqual(threads[0]?.comments.length, 2); + }), ); it.effect("sends a review as its comments, then its summary, then the verdict", () => diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts index ece38c97dbc78..c797e00683ab2 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts @@ -504,7 +504,11 @@ export class GitLabPullRequestCli extends Context.Service< readonly repository: string; readonly number: number; }) => Effect.Effect< - { readonly threads: ReadonlyArray; readonly truncated: boolean }, + { + readonly threads: ReadonlyArray; + readonly overviewFindingIds: ReadonlyArray; + readonly truncated: boolean; + }, GitLabPullRequestCliError >; @@ -959,8 +963,13 @@ export const make = Effect.gen(function* () { readonly number: number; readonly page: number; readonly collected: ReadonlyArray; + readonly collectedFindingIds: ReadonlyArray; }): Effect.Effect< - { readonly threads: ReadonlyArray; readonly truncated: boolean }, + { + readonly threads: ReadonlyArray; + readonly overviewFindingIds: ReadonlyArray; + readonly truncated: boolean; + }, GitLabPullRequestCliError > => api({ @@ -986,13 +995,21 @@ export const make = Effect.gen(function* () { ); } const collected = [...input.collected, ...decoded.success.threads]; + const collectedFindingIds = [ + ...input.collectedFindingIds, + ...decoded.success.overviewFindingIds, + ]; + const done = (truncated: boolean) => + Effect.succeed({ + threads: collected, + overviewFindingIds: collectedFindingIds, + truncated, + }); // Plain notes count toward the page size even when no positioned threads survive decoding. - if (decoded.success.rawCount < MAX_PAGE_SIZE) { - return Effect.succeed({ threads: collected, truncated: false }); - } + if (decoded.success.rawCount < MAX_PAGE_SIZE) return done(false); return input.page >= CONVERSATION_PAGES - ? Effect.succeed({ threads: collected, truncated: true }) - : discussionsPage({ ...input, page: input.page + 1, collected }); + ? done(true) + : discussionsPage({ ...input, page: input.page + 1, collected, collectedFindingIds }); }), ); @@ -2069,7 +2086,7 @@ export const make = Effect.gen(function* () { }), listDiscussions: Effect.fn("GitLabPullRequestCli.listDiscussions")(function (input) { - return discussionsPage({ ...input, page: 1, collected: [] }); + return discussionsPage({ ...input, page: 1, collected: [], collectedFindingIds: [] }); }), submitReview: Effect.fn("GitLabPullRequestCli.submitReview")(function* (input) { diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts index 700b8a9c0aa84..6a39415bf31ed 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts @@ -532,7 +532,8 @@ describe("optional activity reads", () => { }), listNotes: () => Effect.succeed({ comments: [comment], truncated: false }), listCommits: () => Effect.succeed([commit]), - listDiscussions: () => Effect.succeed({ threads: [], truncated: false }), + listDiscussions: () => + Effect.succeed({ threads: [], overviewFindingIds: [], truncated: false }), listReactions: () => Effect.succeed({ reactions: [], @@ -586,6 +587,32 @@ describe("optional activity reads", () => { }), ); + it.effect("turns the first note of an open overview discussion into a review finding", () => + Effect.gen(function* () { + const finding = { ...comment, id: "10", body: "Mention the flag in the README" }; + const reply = { ...comment, id: "11", body: "Will do" }; + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(GitLabPullRequestCli.GitLabPullRequestCli)({ + ...reads, + listNotes: () => + Effect.succeed({ comments: [comment, finding, reply], truncated: false }), + listDiscussions: () => + Effect.succeed({ threads: [], overviewFindingIds: ["10"], truncated: false }), + }), + ), + ); + + const { comments } = yield* provider.getChangeRequestActivity(reference); + + expect(comments.map((item) => [item.id, item.kind])).toEqual([ + ["42", "issue-comment"], + ["10", "review-comment"], + ["11", "issue-comment"], + ]); + }), + ); + it.effect("dates edited comments in the conversation and in threads", () => Effect.gen(function* () { const editedAt = "2026-07-02T00:00:00Z"; @@ -606,6 +633,7 @@ describe("optional activity reads", () => { comments: [comment], }, ], + overviewFindingIds: [], truncated: false, }), listReactions: () => @@ -645,6 +673,7 @@ describe("optional activity reads", () => { comments: [comment], }, ], + overviewFindingIds: [], truncated: false, }), }), @@ -721,6 +750,7 @@ describe("optional activity reads", () => { ], }, ], + overviewFindingIds: [], truncated: false, }), listReactions: () => diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts index 9d2e4a1fc337b..fcf3dbea9fc53 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts @@ -334,7 +334,11 @@ export const make = Effect.gen(function* () { [ recoverRead(cli.listNotes(input), { comments: [], truncated: true }), recoverRead(cli.listCommits(input), []), - recoverRead(cli.listDiscussions(input), { threads: [], truncated: true }), + recoverRead(cli.listDiscussions(input), { + threads: [], + overviewFindingIds: [], + truncated: true, + }), recoverRead(cli.listReactions(input), { reactions: [] as ReadonlyArray, reactionsByNoteId: new Map>(), @@ -353,6 +357,7 @@ export const make = Effect.gen(function* () { Effect.map( ([notes, commits, discussions, awards, webUrl]): ProviderChangeRequestActivity => { const noteUrl = (id: string) => (webUrl === null ? null : `${webUrl}#note_${id}`); + const findingIds = new Set(discussions.overviewFindingIds); const edited = (id: string) => { const editedAt = awards.editedAtByNoteId.get(id); return editedAt === undefined ? {} : { editedAt }; @@ -366,6 +371,7 @@ export const make = Effect.gen(function* () { reactions: awards.reactions, comments: notes.comments.map((comment) => ({ ...comment, + ...(findingIds.has(comment.id) ? { kind: "review-comment" as const } : {}), ...edited(comment.id), author: withBotFlag(comment.author), url: noteUrl(comment.id), diff --git a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts index 1796778aa4912..7d24eff8b7304 100644 --- a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts +++ b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts @@ -553,6 +553,8 @@ export interface GitLabDiffRefs { export interface GitLabDiscussions { readonly threads: ReadonlyArray; + /** First notes of unresolved discussions on the MR itself rather than a diff line. */ + readonly overviewFindingIds: ReadonlyArray; readonly rawCount: number; } @@ -564,12 +566,16 @@ export function decodeDiscussionsJson( return Result.fail(decoded.failure); } const threads: PullRequestReviewThread[] = []; + const overviewFindingIds: string[] = []; for (const entry of decoded.success) { const discussion = decodeDiscussionEntry(entry); if (!Exit.isSuccess(discussion)) continue; const notes = (discussion.value.notes ?? []).filter((note) => note.system !== true); const root = notes[0]; const position = root?.position; + if (root !== undefined && !position && root.resolvable === true && root.resolved !== true) { + overviewFindingIds.push(String(root.id)); + } if (root === undefined || !position) continue; let side: "left" | "right"; let path: string | null; @@ -602,7 +608,7 @@ export function decodeDiscussionsJson( })), }); } - return Result.succeed({ threads, rawCount: decoded.success.length }); + return Result.succeed({ threads, overviewFindingIds, rawCount: decoded.success.length }); } export function decodeDiffRefsJson(