From 40decbc03687cbec451a43f350032392b10ae1fb Mon Sep 17 00:00:00 2001 From: Ossama Al-Alali Date: Thu, 29 Oct 2026 09:42:25 +0100 Subject: [PATCH 1/5] fix(gitlab): merge requests show their approval state GitLab rows never carried a review decision, so the pull request list showed no review badge for merge requests and its Review filter could not find an approved one. Read approvals for listed and summarized merge requests with one GraphQL request per page, using fields both GitLab editions offer. Enterprise Edition's `approved` follows approval rules; Community Edition has none, so an approval there reads as approved and nothing reads as required. A failed read leaves the decision unknown, as before. Co-Authored-By: Claude Opus 5.5 --- .../src/server/GitLabPullRequestCli.ts | 54 ++++++++++ .../server/GitLabPullRequestProvider.test.ts | 99 +++++++++++++++++++ .../src/server/GitLabPullRequestProvider.ts | 41 +++++++- .../src/server/gitLabMergeRequestJson.test.ts | 63 ++++++++++++ .../src/server/gitLabMergeRequestJson.ts | 75 ++++++++++++++ 5 files changed, 330 insertions(+), 2 deletions(-) diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts index 0c1a74827bfa6..c62f21dc58153 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts @@ -19,6 +19,7 @@ import type { PullRequestReaction, PullRequestReactionContent, PullRequestReviewCommentDraft, + PullRequestReviewDecision, PullRequestReviewPosition, PullRequestReviewThread, PullRequestReviewVerdict, @@ -43,9 +44,11 @@ import { decodeProjectLabelsJson, decodeProjectUsersJson, decodeRepositoryBlobsJson, + decodeReviewDecisionsJson, decodeViewerJson, gitLabAwardName, REPOSITORY_BLOBS_GRAPHQL_QUERY, + REVIEW_DECISIONS_GRAPHQL_QUERY, type GitLabDiffRefs, type GitLabMergeRequestDetail, type GitLabMergeRequestListItem, @@ -237,6 +240,15 @@ export class GitLabPullRequestCli extends Context.Service< readonly number: number; }) => Effect.Effect; + readonly getReviewDecisions: (input: { + readonly cwd: string; + readonly repository: string; + readonly numbers: ReadonlyArray; + }) => Effect.Effect< + ReadonlyMap, + GitLabPullRequestCliError + >; + readonly listNotes: (input: { readonly cwd: string; readonly repository: string; @@ -1094,6 +1106,38 @@ export const make = Effect.gen(function* () { }), ); + const reviewDecisionsOf = (input: { + readonly cwd: string; + readonly repository: string; + readonly numbers: ReadonlyArray; + }): Effect.Effect< + ReadonlyMap, + GitLabPullRequestCliError + > => + api({ + cwd: input.cwd, + path: "graphql", + method: "POST", + stdin: JSON.stringify({ + query: REVIEW_DECISIONS_GRAPHQL_QUERY, + variables: { fullPath: input.repository, iids: input.numbers.map(String) }, + }), + }).pipe( + Effect.flatMap((result) => { + const decoded = decodeReviewDecisionsJson(result.stdout.trim()); + return Result.isSuccess(decoded) + ? Effect.succeed(decoded.success) + : Effect.fail( + new GitLabMergeRequestReadError({ + command: "glab", + cwd: input.cwd, + operation: "getReviewDecisions", + cause: decoded.failure, + }), + ); + }), + ); + const viewerUsername = (input: { readonly cwd: string }) => api({ cwd: input.cwd, path: "user" }).pipe( Effect.flatMap((result): Effect.Effect => { @@ -1125,6 +1169,16 @@ export const make = Effect.gen(function* () { getMergeRequestDetail: mergeRequestDetail, + getReviewDecisions: Effect.fn("GitLabPullRequestCli.getReviewDecisions")(function (input) { + const batches: Array> = []; + for (let at = 0; at < input.numbers.length; at += MAX_PAGE_SIZE) { + batches.push(input.numbers.slice(at, at + MAX_PAGE_SIZE)); + } + return Effect.forEach(batches, (numbers) => reviewDecisionsOf({ ...input, numbers }), { + concurrency: 2, + }).pipe(Effect.map((pages) => new Map(pages.flatMap((page) => [...page])))); + }), + listNotes: Effect.fn("GitLabPullRequestCli.listNotes")(function (input) { return notesPage({ ...input, page: 1, collected: [] }); }), diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts index 6eec9ccd09142..b53d058cf3370 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts @@ -73,6 +73,7 @@ describe("getChangeRequest base freshness", () => { Effect.provide( Layer.mock(GitLabPullRequestCli.GitLabPullRequestCli)({ getMergeRequestDetail: () => Effect.succeed(detail), + getReviewDecisions: () => Effect.succeed(new Map()), }), ), ); @@ -221,3 +222,101 @@ describe("optional activity reads", () => { }), ); }); + +describe("review decisions", () => { + const reference = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; + const item = { + number: 7, + title: "Merge request 7", + url: "https://gitlab.com/acme/web/-/merge_requests/7", + author: null, + headBranch: "feat/page", + baseBranch: "main", + state: "open" as const, + isDraft: false, + mergeability: "mergeable" as const, + additions: 0, + deletions: 0, + createdAt: "2026-07-01T00:00:00Z", + updatedAt: "2026-07-02T00:00:00Z", + reviewRequestLogins: [], + labels: [], + }; + const detail = { + ...item, + body: "", + changedFiles: 1, + mergedAt: null, + closedAt: null, + reviewers: [], + checks: [], + viewerCanMerge: true, + reviewerIds: [], + }; + const providerWith = ( + getReviewDecisions: GitLabPullRequestCli.GitLabPullRequestCli["Service"]["getReviewDecisions"], + ) => + make.pipe( + Effect.provide( + Layer.mock(GitLabPullRequestCli.GitLabPullRequestCli)({ + listMergeRequests: () => + Effect.succeed({ + items: [item, { ...item, number: 8 }], + truncated: false, + cursorAdvance: 2, + }), + getMergeRequestDetail: () => Effect.succeed(detail), + getReviewDecisions, + }), + ), + ); + const listing = { + cwd: "/w", + repository: "acme/web", + host: "gitlab.com", + state: "open" as const, + involvement: "all" as const, + viewer: "julius", + limit: 2, + }; + + it.effect("puts GitLab's approval state on listed and summarized merge requests", () => + Effect.gen(function* () { + const provider = yield* providerWith(() => + Effect.succeed( + new Map([ + [7, "approved" as const], + [8, null], + ]), + ), + ); + const listed = yield* provider.listChangeRequests(listing); + assert.isDefined(provider.getChangeRequestSummary); + const summary = yield* provider.getChangeRequestSummary(reference); + + expect(listed.items.map(({ reviewDecision }) => reviewDecision)).toEqual(["approved", null]); + expect(summary.reviewDecision).toBe("approved"); + }), + ); + + it.effect("lists merge requests with an unknown decision when GitLab will not say", () => + Effect.gen(function* () { + const provider = yield* providerWith(() => + Effect.fail( + new GitLabPullRequestCli.GitLabMergeRequestReadError({ + command: "glab", + cwd: "/w", + operation: "getReviewDecisions", + cause: null, + }), + ), + ); + const listed = yield* provider.listChangeRequests(listing); + + expect(listed.items.map(({ reviewDecision }) => reviewDecision)).toEqual([ + undefined, + undefined, + ]); + }), + ); +}); diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts index a9856f907ebb8..4557b9a7f2e7e 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts @@ -3,6 +3,7 @@ import * as Effect from "effect/Effect"; import type { PullRequestCapabilities, PullRequestReaction, + PullRequestReviewDecision, PullRequestViewerPermissions, } from "@t3tools/contracts"; @@ -111,6 +112,9 @@ export const make = Effect.gen(function* () { }), ); + const reviewDecisions = (input: Parameters[0]) => + recoverRead(cli.getReviewDecisions(input), new Map()); + const provider: PullRequestProviderApi = { kind: SourceControlProviderKind.make("gitlab"), capabilities: CAPABILITIES, @@ -133,8 +137,20 @@ export const make = Effect.gen(function* () { filters: input.filters, }) .pipe( + Effect.flatMap((batch) => + reviewDecisions({ + cwd: input.cwd, + repository: input.repository, + numbers: batch.items.map((item) => item.number), + }).pipe( + Effect.map((decisions) => ({ + ...batch, + items: batch.items.map((item) => withReviewDecision(item, decisions)), + continues: true, + })), + ), + ), Effect.mapError(fail("listChangeRequests")), - Effect.map((batch) => ({ ...batch, continues: true })), ), getChangeRequestChecks: (input) => @@ -148,7 +164,20 @@ export const make = Effect.gen(function* () { cli.getMergeRequestDetail(input).pipe(Effect.mapError(fail("getChangeRequestPreview"))), getChangeRequestSummary: (input) => - cli.getMergeRequestDetail(input).pipe(Effect.mapError(fail("getChangeRequestSummary"))), + Effect.all( + [ + cli.getMergeRequestDetail(input), + reviewDecisions({ + cwd: input.cwd, + repository: input.repository, + numbers: [input.number], + }), + ], + { concurrency: 2 }, + ).pipe( + Effect.map(([mergeRequest, decisions]) => withReviewDecision(mergeRequest, decisions)), + Effect.mapError(fail("getChangeRequestSummary")), + ), getChangeRequest: (input) => Effect.all( @@ -330,3 +359,11 @@ export const make = Effect.gen(function* () { return provider; }); + +function withReviewDecision( + item: Item, + decisions: ReadonlyMap, +): Item & { readonly reviewDecision?: PullRequestReviewDecision | null } { + const reviewDecision = decisions.get(item.number); + return reviewDecision === undefined ? item : { ...item, reviewDecision }; +} diff --git a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts index 92f9ed19fe4dc..ca49901f0deae 100644 --- a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts +++ b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts @@ -10,6 +10,7 @@ import { decodeMergeRequestListJson, decodeNotesJson, decodeOwnAwardIdJson, + decodeReviewDecisionsJson, } from "./gitLabMergeRequestJson.ts"; function listJson(entries: ReadonlyArray>): string { @@ -620,3 +621,65 @@ describe("decodeRepositoryBlobsJson", () => { ).toEqual(new Map()); }); }); + +describe("decodeReviewDecisionsJson", () => { + const decisionsOf = (enterprise: boolean, nodes: ReadonlyArray) => + expectSuccess( + decodeReviewDecisionsJson( + JSON.stringify({ + data: { metadata: { enterprise }, project: { mergeRequests: { nodes } } }, + }), + ), + ); + const approvedBy = (...usernames: ReadonlyArray) => ({ + nodes: usernames.map((username) => ({ username })), + }); + + it.each([ + { + reading: "an approval short of Enterprise approval rules", + enterprise: true, + approval: { approved: false, approvedBy: approvedBy("GitLabDuo") }, + decision: "review-required", + }, + { + reading: "Enterprise approval rules no one has approved against", + enterprise: true, + approval: { approved: false, approvedBy: approvedBy() }, + decision: "review-required", + }, + { + reading: "Enterprise approval rules met", + enterprise: true, + approval: { approved: true, approvedBy: approvedBy("julius") }, + decision: "approved", + }, + { + reading: "Enterprise without approval rules or approvals", + enterprise: true, + approval: { approved: true, approvedBy: approvedBy() }, + decision: null, + }, + { + reading: "Community Edition with an approval", + enterprise: false, + approval: { approved: true, approvedBy: approvedBy("julius") }, + decision: "approved", + }, + { + reading: "Community Edition without approvals", + enterprise: false, + approval: { approved: false, approvedBy: approvedBy() }, + decision: null, + }, + ])("reads $reading as $decision", ({ enterprise, approval, decision }) => { + expect([...decisionsOf(enterprise, [{ iid: "7", ...approval }])]).toEqual([[7, decision]]); + }); + + it("leaves out a merge request it cannot key, and a project the reader cannot see", () => { + expect([...decisionsOf(true, [null, { iid: "x", approved: true }])]).toEqual([]); + expect( + expectSuccess(decodeReviewDecisionsJson(JSON.stringify({ data: { project: null } }))), + ).toEqual(new Map()); + }); +}); diff --git a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts index 9c315669d0262..e83c2ed96dd47 100644 --- a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts +++ b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts @@ -15,6 +15,7 @@ import type { PullRequestMergeMethod, PullRequestReaction, PullRequestReactionContent, + PullRequestReviewDecision, PullRequestReviewThread, PullRequestReviewerCandidate, PullRequestState, @@ -946,3 +947,77 @@ export function decodeRepositoryBlobsJson( } return Result.succeed(blobs); } + +export const REVIEW_DECISIONS_GRAPHQL_QUERY = `query($fullPath: ID!, $iids: [String!]) { + metadata { enterprise } + project(fullPath: $fullPath) { + mergeRequests(iids: $iids, first: 100) { + nodes { iid approved approvedBy(first: 1) { nodes { username } } } + } + } +}`; + +const RawReviewDecisionsSchema = Schema.Struct({ + data: Schema.Struct({ + metadata: Schema.optional( + Schema.NullOr(Schema.Struct({ enterprise: Schema.optional(Schema.NullOr(Schema.Boolean)) })), + ), + project: Schema.NullOr( + Schema.Struct({ + mergeRequests: Schema.optional( + Schema.NullOr( + Schema.Struct({ + nodes: Schema.Array( + Schema.NullOr( + Schema.Struct({ + iid: Schema.String, + approved: Schema.optional(Schema.NullOr(Schema.Boolean)), + approvedBy: Schema.optional( + Schema.NullOr( + Schema.Struct({ + nodes: Schema.optional(Schema.NullOr(Schema.Array(Schema.Unknown))), + }), + ), + ), + }), + ), + ), + }), + ), + ), + }), + ), + }), +}); + +const decodeReviewDecisions = decodeJsonResult(RawReviewDecisionsSchema); + +/** + * Only Enterprise Edition holds `approved` to the project's approval rules, so an unapproved + * merge request there still needs review even when someone, such as a bot, approved it. + * Community Edition reports `approved` once anyone approves and requires no review. + */ +export function decodeReviewDecisionsJson( + raw: string, +): Result.Result, DecodeFailure> { + const decoded = decodeReviewDecisions(raw); + if (!Result.isSuccess(decoded)) { + return Result.fail(decoded.failure); + } + const enterprise = decoded.success.data.metadata?.enterprise === true; + const decisions = new Map(); + for (const node of decoded.success.data.project?.mergeRequests?.nodes ?? []) { + const number = Number(node?.iid); + if (node === null || !Number.isSafeInteger(number)) continue; + const approvals = node.approvedBy?.nodes?.length ?? 0; + decisions.set( + number, + node.approved === false && (enterprise || approvals > 0) + ? "review-required" + : approvals > 0 + ? "approved" + : null, + ); + } + return Result.succeed(decisions); +} From 213548d06a78e85495f4791c9e01c7885b66e9ed Mon Sep 17 00:00:00 2001 From: Ossama Al-Alali Date: Thu, 29 Oct 2026 18:23:35 +0100 Subject: [PATCH 2/5] fix(gitlab): a rate-limited approvals read keeps the merge requests The approvals read ran through recoverRead, which rethrows rate limits, so a rate limit on that one extra request failed the whole list or summary. Any failure of it now leaves the decision unknown; the next read still reports a real rate limit. Co-Authored-By: Claude Opus 5.5 --- .../server/GitLabPullRequestProvider.test.ts | 35 +++++++++++++------ .../src/server/GitLabPullRequestProvider.ts | 9 ++++- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts index b53d058cf3370..c2891c8da4419 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts @@ -299,24 +299,37 @@ describe("review decisions", () => { }), ); - it.effect("lists merge requests with an unknown decision when GitLab will not say", () => + it.effect.each([ + { + failure: "a failed read", + error: new GitLabPullRequestCli.GitLabMergeRequestReadError({ + command: "glab", + cwd: "/w", + operation: "getReviewDecisions", + cause: null, + }), + }, + { + failure: "a rate limit", + error: new GitLabCli.GitLabCliRateLimitError({ + operation: "execute", + command: "glab", + cwd: "/w", + cause: null, + }), + }, + ])("keeps rows and summaries with an unknown decision after $failure", ({ error }) => Effect.gen(function* () { - const provider = yield* providerWith(() => - Effect.fail( - new GitLabPullRequestCli.GitLabMergeRequestReadError({ - command: "glab", - cwd: "/w", - operation: "getReviewDecisions", - cause: null, - }), - ), - ); + const provider = yield* providerWith(() => Effect.fail(error)); const listed = yield* provider.listChangeRequests(listing); + assert.isDefined(provider.getChangeRequestSummary); + const summary = yield* provider.getChangeRequestSummary(reference); expect(listed.items.map(({ reviewDecision }) => reviewDecision)).toEqual([ undefined, undefined, ]); + expect(summary.reviewDecision).toBeUndefined(); }), ); }); diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts index 4557b9a7f2e7e..d72f5e91c6e7f 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts @@ -112,8 +112,15 @@ export const make = Effect.gen(function* () { }), ); + // The rows were already read, so even a rate limit only leaves their decision unknown. const reviewDecisions = (input: Parameters[0]) => - recoverRead(cli.getReviewDecisions(input), new Map()); + cli + .getReviewDecisions(input) + .pipe( + Effect.orElseSucceed( + (): ReadonlyMap => new Map(), + ), + ); const provider: PullRequestProviderApi = { kind: SourceControlProviderKind.make("gitlab"), From 3fa2e542a825eb096e75d3c559a86b9706c49e7c Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Fri, 30 Oct 2026 05:10:59 +0000 Subject: [PATCH 3/5] fix(gitlab): keep incomplete approval responses unknown --- .../src/server/GitLabPullRequestCli.test.ts | 50 +++++++++++++++++++ .../src/server/gitLabMergeRequestJson.test.ts | 6 +++ .../src/server/gitLabMergeRequestJson.ts | 10 +++- 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts index 16326f57ffecd..1e2404943b0bb 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts @@ -96,6 +96,56 @@ afterEach(() => { }); layer("GitLabPullRequestCli.layer", (it) => { + it.effect("batches approval reads at GitLab's page limit and skips an empty list", () => + Effect.gen(function* () { + mockedExecute.mockImplementation(({ stdin }) => { + const { variables } = JSON.parse(stdin ?? "{}"); + return Effect.succeed( + output( + JSON.stringify({ + data: { + metadata: { enterprise: true }, + project: { + mergeRequests: { + nodes: variables.iids.map((iid: string) => ({ + iid, + approved: true, + approvedBy: { nodes: [{ username: "reviewer" }] }, + })), + }, + }, + }, + }), + ), + ); + }); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + const reference = { cwd: "/w", repository: "acme/web" }; + expect(yield* cli.getReviewDecisions({ ...reference, numbers: [] })).toEqual(new Map()); + expect(mockedExecute).not.toHaveBeenCalled(); + const decisions = yield* cli.getReviewDecisions({ + ...reference, + numbers: Array.from({ length: 101 }, (_, index) => index + 1), + }); + expect(decisions.size).toBe(101); + expect(decisions.get(101)).toBe("approved"); + expect(mockedExecute).toHaveBeenCalledTimes(2); + expect( + [0, 1].map((index) => JSON.parse(callAt(index).stdin ?? "{}").variables.iids.length), + ).toEqual([100, 1]); + expect(argsOfCall(0)).toEqual([ + "api", + "graphql", + "--method", + "POST", + "--input", + "-", + "--header", + "Content-Type: application/json", + ]); + }), + ); + it.effect("asks GitLab for one row more than the page, to probe for a next page", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce(Effect.succeed(output(mergeRequests(3, 1)))); diff --git a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts index ca49901f0deae..09b9e680c9374 100644 --- a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts +++ b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts @@ -678,6 +678,12 @@ describe("decodeReviewDecisionsJson", () => { it("leaves out a merge request it cannot key, and a project the reader cannot see", () => { expect([...decisionsOf(true, [null, { iid: "x", approved: true }])]).toEqual([]); + expect([ + ...decisionsOf(true, [ + { iid: "7", approved: null, approvedBy: approvedBy("julius") }, + { iid: "8", approved: true, approvedBy: null }, + ]), + ]).toEqual([]); expect( expectSuccess(decodeReviewDecisionsJson(JSON.stringify({ data: { project: null } }))), ).toEqual(new Map()); diff --git a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts index e83c2ed96dd47..378160dadd4ef 100644 --- a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts +++ b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts @@ -1008,8 +1008,14 @@ export function decodeReviewDecisionsJson( const decisions = new Map(); for (const node of decoded.success.data.project?.mergeRequests?.nodes ?? []) { const number = Number(node?.iid); - if (node === null || !Number.isSafeInteger(number)) continue; - const approvals = node.approvedBy?.nodes?.length ?? 0; + if ( + node === null || + !Number.isSafeInteger(number) || + node.approved == null || + node.approvedBy?.nodes == null + ) + continue; + const approvals = node.approvedBy.nodes.length; decisions.set( number, node.approved === false && (enterprise || approvals > 0) From 442db266267899434bd26dcbd1fda1ee2600b0e5 Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Fri, 30 Oct 2026 05:15:47 +0000 Subject: [PATCH 4/5] fix(gitlab): preserve unknown approval state when edition is hidden --- .../src/server/gitLabMergeRequestJson.test.ts | 10 ++++++++-- .../src/server/gitLabMergeRequestJson.ts | 3 ++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts index 09b9e680c9374..1b4ea5d05e15f 100644 --- a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts +++ b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.test.ts @@ -623,11 +623,14 @@ describe("decodeRepositoryBlobsJson", () => { }); describe("decodeReviewDecisionsJson", () => { - const decisionsOf = (enterprise: boolean, nodes: ReadonlyArray) => + const decisionsOf = (enterprise: boolean | null, nodes: ReadonlyArray) => expectSuccess( decodeReviewDecisionsJson( JSON.stringify({ - data: { metadata: { enterprise }, project: { mergeRequests: { nodes } } }, + data: { + metadata: enterprise === null ? null : { enterprise }, + project: { mergeRequests: { nodes } }, + }, }), ), ); @@ -678,6 +681,9 @@ describe("decodeReviewDecisionsJson", () => { it("leaves out a merge request it cannot key, and a project the reader cannot see", () => { expect([...decisionsOf(true, [null, { iid: "x", approved: true }])]).toEqual([]); + expect([ + ...decisionsOf(null, [{ iid: "7", approved: false, approvedBy: approvedBy() }]), + ]).toEqual([]); expect([ ...decisionsOf(true, [ { iid: "7", approved: null, approvedBy: approvedBy("julius") }, diff --git a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts index 378160dadd4ef..8d13dd53958f1 100644 --- a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts +++ b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts @@ -1004,7 +1004,7 @@ export function decodeReviewDecisionsJson( if (!Result.isSuccess(decoded)) { return Result.fail(decoded.failure); } - const enterprise = decoded.success.data.metadata?.enterprise === true; + const enterprise = decoded.success.data.metadata?.enterprise; const decisions = new Map(); for (const node of decoded.success.data.project?.mergeRequests?.nodes ?? []) { const number = Number(node?.iid); @@ -1016,6 +1016,7 @@ export function decodeReviewDecisionsJson( ) continue; const approvals = node.approvedBy.nodes.length; + if (node.approved === false && approvals === 0 && enterprise == null) continue; decisions.set( number, node.approved === false && (enterprise || approvals > 0) From 428d8a561e56318b9382e9692b6ff8db7228fc3e Mon Sep 17 00:00:00 2001 From: maria-rcks Date: Fri, 30 Oct 2026 21:51:01 +0000 Subject: [PATCH 5/5] test(gitlab): account for the optional approval read --- .../src/server/GitLabPullRequestProvider.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts index 597a62ebf457b..dbf25b531a23a 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts @@ -112,7 +112,7 @@ it.effect.each(["merge", "disable-auto-merge"] as const)( yield* provider.searchRepositoryAuthors(reference); yield* provider.runAction({ ...reference, action }); - expect(readHosts).toEqual(Array(action === "merge" ? 11 : 10).fill(selectedHost)); + expect(readHosts).toEqual(Array(action === "merge" ? 12 : 11).fill(selectedHost)); expect({ url: detail.url, mutatedHosts: mergedHosts }).toEqual({ url: `https://${selectedHost}/acme/web/-/merge_requests/7`, mutatedHosts: [selectedHost],