History

fix(server): retain GitLab approval system notes

Julius Marminge committed 2 months ago

Commit 33037eb1 parent ce4712d3 files · +146 −11Browse files

Unified · Split

3 files
  1. apps/server/src/pullRequest/GitLabPullRequestCli.test.ts +58 −0
  2. apps/server/src/pullRequest/gitLabMergeRequestJson.test.ts +65 −0
  3. apps/server/src/pullRequest/gitLabMergeRequestJson.ts +23 −11
apps/server/src/pullRequest/GitLabPullRequestCli.test.ts+58−0
@@ -913,6 +913,64 @@ layer("GitLabPullRequestCli.layer", (it) => { · 912 unmodified lines
913913 }),
914914 );
915915
916+ it.effect("reads approval history after a full page of unrelated system notes", () =>
917+ Effect.gen(function* () {
918+ mockedExecute.mockReturnValueOnce(
919+ Effect.succeed(
920+ output(
921+ // @effect-diagnostics-next-line preferSchemaOverJson:off
922+ JSON.stringify(
923+ Array.from({ length: 100 }, (_, index) => ({
924+ id: index + 1,
925+ system: true,
926+ body: "assigned to @reviewer",
927+ created_at: "2026-07-01T00:00:00Z",
928+ })),
929+ ),
930+ ),
931+ ),
932+ );
933+ mockedExecute.mockReturnValueOnce(
934+ Effect.succeed(
935+ output(
936+ // @effect-diagnostics-next-line preferSchemaOverJson:off
937+ JSON.stringify([
938+ {
939+ id: 101,
940+ system: true,
941+ body: "approved this merge request",
942+ author: { username: "reviewer" },
943+ created_at: "2026-07-02T00:00:00Z",
944+ },
945+ {
946+ id: 102,
947+ system: true,
948+ body: "unapproved this merge request",
949+ author: { username: "reviewer" },
950+ created_at: "2026-07-03T00:00:00Z",
951+ },
952+ ]),
953+ ),
954+ ),
955+ );
956+
957+ const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli;
958+ const { comments, truncated } = yield* cli.listNotes({
959+ cwd: "/w",
960+ repository: "acme/web",
961+ number: 7,
962+ });
963+
964+ expect(comments).toMatchObject([
965+ { id: "101", kind: "review", reviewState: "APPROVED" },
966+ { id: "102", kind: "review", reviewState: "DISMISSED" },
967+ ]);
968+ expect(argsOfCall(1).join(" ")).toContain("page=2");
969+ assert.strictEqual(mockedExecute.mock.calls.length, 2);
970+ assert.isFalse(truncated);
971+ }),
972+ );
973+
916974 it.effect("stops the note walk at its bound and says the conversation was cut short", () =>
917975 Effect.gen(function* () {
918976 // GitLab that never answers short: the walk has to end itself.
apps/server/src/pullRequest/gitLabMergeRequestJson.test.ts+65−0
@@ -247,6 +247,71 @@ describe("decodeViewerJson", () => { · 246 unmodified lines
247247 });
248248
249249 describe("decodeNotesJson", () => {
250+ it("retains approval and withdrawal notes without classifying authored comments as verdicts", () => {
251+ const notes = expectSuccess(
252+ decodeNotesJson(
253+ JSON.stringify([
254+ {
255+ id: 1,
256+ body: "approved this merge request",
257+ author: { username: "reviewer" },
258+ system: true,
259+ created_at: "2026-07-01T00:00:00Z",
260+ },
261+ {
262+ id: 2,
263+ body: "unapproved this merge request",
264+ author: { username: "reviewer" },
265+ system: true,
266+ created_at: "2026-07-02T00:00:00Z",
267+ },
268+ {
269+ id: 3,
270+ body: "approved this merge request",
271+ author: { username: "author" },
272+ system: false,
273+ created_at: "2026-07-03T00:00:00Z",
274+ },
275+ {
276+ id: 4,
277+ body: "assigned to @reviewer",
278+ system: true,
279+ created_at: "2026-07-04T00:00:00Z",
280+ },
281+ {
282+ id: 5,
283+ body: "approved this merge request with a custom message",
284+ system: true,
285+ created_at: "2026-07-05T00:00:00Z",
286+ },
287+ ]),
288+ ),
289+ );
290+
291+ expect(notes.rawCount).toBe(5);
292+ expect(notes.comments).toMatchObject([
293+ {
294+ id: "1",
295+ kind: "review",
296+ body: "approved this merge request",
297+ author: { login: "reviewer" },
298+ createdAt: "2026-07-01T00:00:00Z",
299+ reviewState: "APPROVED",
300+ path: null,
301+ },
302+ {
303+ id: "2",
304+ kind: "review",
305+ body: "unapproved this merge request",
306+ author: { login: "reviewer" },
307+ createdAt: "2026-07-02T00:00:00Z",
308+ reviewState: "DISMISSED",
309+ path: null,
310+ },
311+ { id: "3", kind: "issue-comment", reviewState: null },
312+ ]);
313+ });
314+
250315 it("keeps comments and drops GitLab's own activity notes", () => {
251316 const notes = expectSuccess(
252317 decodeNotesJson(
apps/server/src/pullRequest/gitLabMergeRequestJson.ts+23−11
@@ -515,13 +515,6 @@ export function decodeProjectMergeCapabilitiesJson( · 514 unmodified lines
515515 });
516516 }
517517
518−/**
519− * Comments only. System notes are GitLab's own activity feed entries, and a `DiffNote` is the
520− * root of a line-level discussion, which is what the review-comment kind means.
521− *
522− * The raw note count comes back alongside, because dropping notes hides whether the page was
523− * full: a caller cannot tell "no more notes" from "a page of activity entries" without it.
524− */
525518 /** The three revisions a positioned comment is written against. */
526519 export interface GitLabDiffRefs {
527520 readonly baseSha: string;
@@ -594,6 +587,21 @@ export function decodeDiffRefsJson( · 66 unmodified lines
594587 );
595588 }
596589
590+function systemNoteReviewState(body: string): "APPROVED" | "DISMISSED" | null {
591+ switch (body.trim()) {
592+ case "approved this merge request":
593+ return "APPROVED";
594+ case "unapproved this merge request":
595+ return "DISMISSED";
596+ default:
597+ return null;
598+ }
599+}
600+
601+/**
602+ * Comments and approval history. Other system activity stays excluded. The raw count includes
603+ * every note so a page containing only skipped activity does not end pagination early.
604+ */
597605 export function decodeNotesJson(
598606 raw: string,
599607 ): Result.Result<
@@ -609,19 +617,23 @@ export function decodeNotesJson( · 9 unmodified lines
609617 const note = decodeNoteEntry(entry);
610618 if (Exit.isFailure(note)) continue;
611619 const value = note.value;
612− if (value.system === true) continue;
613620 const body = value.body ?? "";
621+ const reviewState = value.system === true ? systemNoteReviewState(body) : null;
622+ if (value.system === true && reviewState === null) continue;
614623 if (body.trim().length === 0) continue;
615624 const isDiffNote = value.type?.trim() === "DiffNote";
616625 comments.push({
617626 id: String(value.id),
618− kind: isDiffNote ? "review-comment" : "issue-comment",
627+ kind: reviewState !== null ? "review" : isDiffNote ? "review-comment" : "issue-comment",
619628 author: toActor(value.author),
620629 body,
621630 createdAt: value.created_at,
622631 url: null,
623− path: trimmed(value.position?.new_path) ?? trimmed(value.position?.old_path),
624− reviewState: null,
632+ path:
633+ reviewState !== null
634+ ? null
635+ : (trimmed(value.position?.new_path) ?? trimmed(value.position?.old_path)),
636+ reviewState,
625637 });
626638 }
627639 return Result.succeed({ comments, rawCount: decoded.success.length });