Skip to content

fix(gitlab): edited comments wake PR watchers #30403

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 2 commits into from
Nov 6, 2026
Merged

fix(gitlab): edited comments wake PR watchers #30403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b8fa652
fix(gitlab): edited comments wake PR watchers
TonybynMp4 Nov 3, 2026
82fec15
Merge main into fix/gitlab-edited-comments
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
6 changes: 6 additions & 0 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 @@ -466,6 +466,7 @@ export class GitLabPullRequestCli extends Context.Service<
{
readonly reactions: ReadonlyArray<PullRequestReaction>;
readonly reactionsByNoteId: ReadonlyMap<string, ReadonlyArray<PullRequestReaction>>;
readonly editedAtByNoteId: ReadonlyMap<string, string>;
readonly botLogins: ReadonlySet<string>;
},
GitLabPullRequestCliError
Expand Down Expand Up @@ -1092,12 +1093,14 @@ export const make = Effect.gen(function* () {
readonly collected: {
readonly reactions: ReadonlyArray<PullRequestReaction>;
readonly reactionsByNoteId: Map<string, ReadonlyArray<PullRequestReaction>>;
readonly editedAtByNoteId: Map<string, string>;
readonly botLogins: Set<string>;
} | null;
}): Effect.Effect<
{
readonly reactions: ReadonlyArray<PullRequestReaction>;
readonly reactionsByNoteId: ReadonlyMap<string, ReadonlyArray<PullRequestReaction>>;
readonly editedAtByNoteId: ReadonlyMap<string, string>;
readonly botLogins: ReadonlySet<string>;
},
GitLabPullRequestCliError
Expand Down Expand Up @@ -1131,10 +1134,13 @@ export const make = Effect.gen(function* () {
const collected = input.collected ?? {
reactions: decoded.success.reactions,
reactionsByNoteId: new Map<string, ReadonlyArray<PullRequestReaction>>(),
editedAtByNoteId: new Map<string, string>(),
botLogins: new Set<string>(),
};
for (const [id, reactions] of decoded.success.reactionsByNoteId)
collected.reactionsByNoteId.set(id, reactions);
for (const [id, editedAt] of decoded.success.editedAtByNoteId)
collected.editedAtByNoteId.set(id, editedAt);
for (const login of decoded.success.botLogins) collected.botLogins.add(login);
return decoded.success.nextCursor === null || input.page >= CONVERSATION_PAGES
? Effect.succeed(collected)
Expand Down
48 changes: 47 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 @@ -485,7 +485,12 @@ describe("optional activity reads", () => {
listCommits: () => Effect.succeed([commit]),
listDiscussions: () => Effect.succeed({ threads: [], truncated: false }),
listReactions: () =>
Effect.succeed({ reactions: [], reactionsByNoteId: new Map(), botLogins: new Set<string>() }),
Effect.succeed({
reactions: [],
reactionsByNoteId: new Map(),
editedAtByNoteId: new Map(),
botLogins: new Set<string>(),
}),
};
const context = {
operation: "execute" as const,
Expand Down Expand Up @@ -532,6 +537,46 @@ describe("optional activity reads", () => {
}),
);

it.effect("dates edited comments in the conversation and in threads", () =>
Effect.gen(function* () {
const editedAt = "2026-07-02T00:00:00Z";
const provider = yield* make.pipe(
Effect.provide(
Layer.mock(GitLabPullRequestCli.GitLabPullRequestCli)({
...reads,
listDiscussions: () =>
Effect.succeed({
threads: [
{
id: "d1",
path: "src/a.ts",
line: 1,
side: "right" as const,
isResolved: false,
isOutdated: false,
comments: [comment],
},
],
truncated: false,
}),
listReactions: () =>
Effect.succeed({
reactions: [],
reactionsByNoteId: new Map(),
editedAtByNoteId: new Map([[comment.id, editedAt]]),
botLogins: new Set<string>(),
}),
}),
),
);

const activity = yield* provider.getChangeRequestActivity(reference);

expect(activity.comments[0]?.editedAt).toBe(editedAt);
expect(activity.reviewThreads[0]?.comments[0]?.editedAt).toBe(editedAt);
}),
);

it.effect("keeps comments and commits when optional discussions and reactions fail", () =>
Effect.gen(function* () {
const provider = yield* make.pipe(
Expand Down Expand Up @@ -600,6 +645,7 @@ describe("optional activity reads", () => {
Effect.succeed({
reactions: [],
reactionsByNoteId: new Map(),
editedAtByNoteId: new Map(),
botLogins: new Set([bot.login.toUpperCase()]),
}),
}),
Expand Down
7 changes: 7 additions & 0 deletions 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 @@ -331,13 +331,18 @@ export const make = Effect.gen(function* () {
recoverRead(cli.listReactions(input), {
reactions: [] as ReadonlyArray<PullRequestReaction>,
reactionsByNoteId: new Map<string, ReadonlyArray<PullRequestReaction>>(),
editedAtByNoteId: new Map<string, string>(),
botLogins: new Set<string>(),
}),
],
{ concurrency: 4 },
).pipe(
Effect.mapError(fail("getChangeRequestActivity")),
Effect.map(([notes, commits, discussions, awards]): ProviderChangeRequestActivity => {
const edited = (id: string) => {
const editedAt = awards.editedAtByNoteId.get(id);
return editedAt === undefined ? {} : { editedAt };
};
const botLogins = new Set([...awards.botLogins].map((login) => login.toLowerCase()));
const withBotFlag = (author: PullRequestActor | null) =>
author !== null && botLogins.has(author.login.toLowerCase())
Expand All @@ -347,6 +352,7 @@ export const make = Effect.gen(function* () {
reactions: awards.reactions,
comments: notes.comments.map((comment) => ({
...comment,
...edited(comment.id),
author: withBotFlag(comment.author),
reactions: awards.reactionsByNoteId.get(comment.id) ?? [],
})),
Expand All @@ -356,6 +362,7 @@ export const make = Effect.gen(function* () {
...thread,
comments: thread.comments.map((comment) => ({
...comment,
...edited(comment.id),
author: withBotFlag(comment.author),
reactions: awards.reactionsByNoteId.get(comment.id) ?? [],
})),
Expand Down
42 changes: 42 additions & 0 deletions packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,48 @@ describe("decodeAwardEmojiJson", () => {
expect(result.nextCursor).toBeNull();
});

it("dates a note's edit only when someone edited its text, not when its thread was resolved", () => {
const result = expectSuccess(
decodeAwardEmojiJson(
JSON.stringify({
data: {
currentUser: { username: "bilal" },
project: {
mergeRequest: {
awardEmoji: { nodes: [] },
notes: {
pageInfo: { hasNextPage: false, endCursor: null },
nodes: [
{
id: "gid://gitlab/Note/1",
lastEditedAt: "2026-07-02T00:00:00Z",
lastEditedBy: { username: "julius" },
resolved: false,
},
{
id: "gid://gitlab/Note/2",
lastEditedAt: "2026-07-01T00:00:00Z",
lastEditedBy: null,
resolved: false,
},
{
id: "gid://gitlab/DiscussionNote/3",
lastEditedAt: "2026-07-03T00:00:00Z",
lastEditedBy: { username: "julius" },
resolved: true,
},
],
},
},
},
},
}),
),
);

expect([...result.editedAtByNoteId]).toEqual([["1", "2026-07-02T00:00:00Z"]]);
});

it("matches the viewer's username case-insensitively, since GitLab is not consistent about case", () => {
const result = expectSuccess(
decodeAwardEmojiJson(
Expand Down
24 changes: 23 additions & 1 deletion packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,7 +780,7 @@ export const AWARD_EMOJI_GRAPHQL_QUERY = `query($fullPath: ID!, $iid: String!, $
awardEmoji { nodes { name user { username } } }
notes(first: 100, after: $cursor) {
pageInfo { hasNextPage endCursor }
nodes { id author { username bot } awardEmoji { nodes { name user { username } } } }
nodes { id author { username bot } lastEditedAt lastEditedBy { username } resolved awardEmoji { nodes { name user { username } } } }
}
}
}
Expand Down Expand Up @@ -832,6 +832,15 @@ const RawAwardEmojiPageSchema = Schema.Struct({
Schema.NullOr(
Schema.Struct({
id: Schema.optional(Schema.NullOr(Schema.String)),
lastEditedAt: Schema.optional(Schema.NullOr(Schema.String)),
lastEditedBy: Schema.optional(
Schema.NullOr(
Schema.Struct({
username: Schema.optional(Schema.NullOr(Schema.String)),
}),
),
),
resolved: Schema.optional(Schema.NullOr(Schema.Boolean)),
// REST note authors carry no bot flag; GraphQL's does.
author: Schema.optional(
Schema.NullOr(
Expand Down Expand Up @@ -895,6 +904,7 @@ function noteIdOf(gid: string | null | undefined): string | null {
export interface GitLabAwardEmojiPage {
readonly reactions: ReadonlyArray<PullRequestReaction>;
readonly reactionsByNoteId: ReadonlyMap<string, ReadonlyArray<PullRequestReaction>>;
readonly editedAtByNoteId: ReadonlyMap<string, string>;
readonly botLogins: ReadonlySet<string>;
readonly nextCursor: string | null;
}
Expand All @@ -910,6 +920,7 @@ export function decodeAwardEmojiJson(
const viewer = trimmed(data.currentUser?.username);
const mergeRequest = data.project?.mergeRequest;
const reactionsByNoteId = new Map<string, ReadonlyArray<PullRequestReaction>>();
const editedAtByNoteId = new Map<string, string>();
const botLogins = new Set<string>();
for (const node of mergeRequest?.notes?.nodes ?? []) {
const author = trimmed(node?.author?.username);
Expand All @@ -918,11 +929,22 @@ export function decodeAwardEmojiJson(
if (id === null) continue;
const reactions = toReactions(node?.awardEmoji, viewer);
if (reactions.length > 0) reactionsByNoteId.set(id, reactions);
// Resolving a thread moves lastEditedAt on every note in it; only lastEditedBy marks a
// text edit, and a resolved note's stamp may be the resolve rather than that edit.
const editedAt = trimmed(node?.lastEditedAt);
if (
editedAt !== null &&
trimmed(node?.lastEditedBy?.username) !== null &&
node?.resolved !== true
) {
editedAtByNoteId.set(id, editedAt);
}
}
const pageInfo = mergeRequest?.notes?.pageInfo;
return Result.succeed({
reactions: toReactions(mergeRequest?.awardEmoji, viewer),
reactionsByNoteId,
editedAtByNoteId,
botLogins,
nextCursor: pageInfo?.hasNextPage === true ? (trimmed(pageInfo.endCursor) ?? null) : null,
});
Expand Down
Loading