Skip to content

fix(gitlab): unresolved overview threads count as review findings #30397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
juliusmarminge merged 3 commits into from
Nov 6, 2026
Merged

fix(gitlab): unresolved overview threads count as review findings #30397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b69791b
fix(gitlab): unresolved overview threads count as review findings
TonybynMp4 Nov 3, 2026
f95bf5d
Merge pingdotgg/main into fix/gitlab-overview-thread-findings
TonybynMp4 Nov 5, 2026
f2ff6ab
Merge main into fix/gitlab-overview-thread-findings
juliusmarminge Nov 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 115 additions & 81 deletions packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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", () =>
Expand Down
33 changes: 25 additions & 8 deletions packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,11 @@ export class GitLabPullRequestCli extends Context.Service<
readonly repository: string;
readonly number: number;
}) => Effect.Effect<
{ readonly threads: ReadonlyArray<PullRequestReviewThread>; readonly truncated: boolean },
{
readonly threads: ReadonlyArray<PullRequestReviewThread>;
readonly overviewFindingIds: ReadonlyArray<string>;
readonly truncated: boolean;
},
GitLabPullRequestCliError
>;

Expand Down Expand Up @@ -959,8 +963,13 @@ export const make = Effect.gen(function* () {
readonly number: number;
readonly page: number;
readonly collected: ReadonlyArray<PullRequestReviewThread>;
readonly collectedFindingIds: ReadonlyArray<string>;
}): Effect.Effect<
{ readonly threads: ReadonlyArray<PullRequestReviewThread>; readonly truncated: boolean },
{
readonly threads: ReadonlyArray<PullRequestReviewThread>;
readonly overviewFindingIds: ReadonlyArray<string>;
readonly truncated: boolean;
},
GitLabPullRequestCliError
> =>
api({
Expand All @@ -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 });
}),
);

Expand Down Expand Up @@ -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) {
Expand Down
32 changes: 31 additions & 1 deletion packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: [],
Expand Down Expand Up @@ -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";
Expand All @@ -606,6 +633,7 @@ describe("optional activity reads", () => {
comments: [comment],
},
],
overviewFindingIds: [],
truncated: false,
}),
listReactions: () =>
Expand Down Expand Up @@ -645,6 +673,7 @@ describe("optional activity reads", () => {
comments: [comment],
},
],
overviewFindingIds: [],
truncated: false,
}),
}),
Expand Down Expand Up @@ -721,6 +750,7 @@ describe("optional activity reads", () => {
],
},
],
overviewFindingIds: [],
truncated: false,
}),
listReactions: () =>
Expand Down
8 changes: 7 additions & 1 deletion packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<PullRequestReaction>,
reactionsByNoteId: new Map<string, ReadonlyArray<PullRequestReaction>>(),
Expand All @@ -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 };
Expand All @@ -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),
Expand Down
Loading
Loading