From b8fa6528bff3b18463409ba167a45e49ab49915f Mon Sep 17 00:00:00 2001 From: Tony <97451137+TonybynMp4@users.noreply.github.com> Date: Tue, 3 Nov 2026 22:28:27 +0100 Subject: [PATCH] fix(gitlab): edited comments wake PR watchers GitLab comments never carried editedAt, so a bot that rewrites its summary comment in place never woke watch_pull_request. The reactions GraphQL query, which already walks every note, now also reads lastEditedAt, lastEditedBy and resolved. Resolving a thread moves lastEditedAt on all its notes, so the edit time is trusted only when lastEditedBy is set and the note is not resolved. Co-Authored-By: Claude Opus 5.5 --- .../src/server/GitLabPullRequestCli.ts | 6 +++ .../server/GitLabPullRequestProvider.test.ts | 42 ++++++++++++++++++- .../src/server/GitLabPullRequestProvider.ts | 37 +++++++++------- .../src/server/gitLabMergeRequestJson.test.ts | 42 +++++++++++++++++++ .../src/server/gitLabMergeRequestJson.ts | 24 ++++++++++- 5 files changed, 135 insertions(+), 16 deletions(-) diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts index ba8710f82f535..577b6731244fd 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts @@ -461,6 +461,7 @@ export class GitLabPullRequestCli extends Context.Service< { readonly reactions: ReadonlyArray; readonly reactionsByNoteId: ReadonlyMap>; + readonly editedAtByNoteId: ReadonlyMap; }, GitLabPullRequestCliError >; @@ -1086,11 +1087,13 @@ export const make = Effect.gen(function* () { readonly collected: { readonly reactions: ReadonlyArray; readonly reactionsByNoteId: Map>; + readonly editedAtByNoteId: Map; } | null; }): Effect.Effect< { readonly reactions: ReadonlyArray; readonly reactionsByNoteId: ReadonlyMap>; + readonly editedAtByNoteId: ReadonlyMap; }, GitLabPullRequestCliError > => @@ -1123,9 +1126,12 @@ export const make = Effect.gen(function* () { const collected = input.collected ?? { reactions: decoded.success.reactions, reactionsByNoteId: new Map>(), + editedAtByNoteId: new Map(), }; 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); return decoded.success.nextCursor === null || input.page >= CONVERSATION_PAGES ? Effect.succeed(collected) : awardsPage({ diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts index dbf25b531a23a..b23b86fd01800 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts @@ -484,7 +484,8 @@ describe("optional activity reads", () => { listNotes: () => Effect.succeed({ comments: [comment], truncated: false }), listCommits: () => Effect.succeed([commit]), listDiscussions: () => Effect.succeed({ threads: [], truncated: false }), - listReactions: () => Effect.succeed({ reactions: [], reactionsByNoteId: new Map() }), + listReactions: () => + Effect.succeed({ reactions: [], reactionsByNoteId: new Map(), editedAtByNoteId: new Map() }), }; const context = { operation: "execute" as const, @@ -531,6 +532,45 @@ 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]]), + }), + }), + ), + ); + + 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( diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts index 9935bf17a4b37..65786bcf4c789 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts @@ -328,28 +328,37 @@ export const make = Effect.gen(function* () { recoverRead(cli.listReactions(input), { reactions: [] as ReadonlyArray, reactionsByNoteId: new Map>(), + editedAtByNoteId: new Map(), }), ], { concurrency: 4 }, ).pipe( Effect.mapError(fail("getChangeRequestActivity")), - Effect.map(([notes, commits, discussions, awards]): ProviderChangeRequestActivity => ({ - reactions: awards.reactions, - comments: notes.comments.map((comment) => ({ - ...comment, - reactions: awards.reactionsByNoteId.get(comment.id) ?? [], - })), - commentCount: notes.comments.length, - commentsTruncated: notes.truncated || discussions.truncated, - reviewThreads: discussions.threads.map((thread) => ({ - ...thread, - comments: thread.comments.map((comment) => ({ + Effect.map(([notes, commits, discussions, awards]): ProviderChangeRequestActivity => { + const edited = (id: string) => { + const editedAt = awards.editedAtByNoteId.get(id); + return editedAt === undefined ? {} : { editedAt }; + }; + return { + reactions: awards.reactions, + comments: notes.comments.map((comment) => ({ ...comment, + ...edited(comment.id), reactions: awards.reactionsByNoteId.get(comment.id) ?? [], })), - })), - commits, - })), + commentCount: notes.comments.length, + commentsTruncated: notes.truncated || discussions.truncated, + reviewThreads: discussions.threads.map((thread) => ({ + ...thread, + comments: thread.comments.map((comment) => ({ + ...comment, + ...edited(comment.id), + reactions: awards.reactionsByNoteId.get(comment.id) ?? [], + })), + })), + commits, + }; + }), ), // Straight from the host, never the shared read: this is what a write is refused or let diff --git a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts index 371c2e3f3c70a..489951cfd3c20 100644 --- a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts +++ b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts @@ -539,6 +539,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( diff --git a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts index 79ab379ffbd44..a63e6a1f85483 100644 --- a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts +++ b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts @@ -767,7 +767,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 awardEmoji { nodes { name user { username } } } } + nodes { id lastEditedAt lastEditedBy { username } resolved awardEmoji { nodes { name user { username } } } } } } } @@ -819,6 +819,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)), awardEmoji: RawAwardEmojiNodesSchema, }), ), @@ -873,6 +882,7 @@ function noteIdOf(gid: string | null | undefined): string | null { export interface GitLabAwardEmojiPage { readonly reactions: ReadonlyArray; readonly reactionsByNoteId: ReadonlyMap>; + readonly editedAtByNoteId: ReadonlyMap; readonly nextCursor: string | null; } @@ -887,16 +897,28 @@ export function decodeAwardEmojiJson( const viewer = trimmed(data.currentUser?.username); const mergeRequest = data.project?.mergeRequest; const reactionsByNoteId = new Map>(); + const editedAtByNoteId = new Map(); for (const node of mergeRequest?.notes?.nodes ?? []) { const id = noteIdOf(node?.id); 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, nextCursor: pageInfo?.hasNextPage === true ? (trimmed(pageInfo.endCursor) ?? null) : null, }); }