From 31df44f949eb213409a6925f75841645c9bd0b13 Mon Sep 17 00:00:00 2001 From: ScottN-PV Date: Fri, 9 Oct 2026 11:00:15 -0400 Subject: [PATCH] fix(server): GitLab merge requests can expand unchanged lines The GitLab provider never wired the diff file contents read, so the pull request service refused every expand with "This host cannot expand unchanged pull request lines." The CLI read already existed; the provider now exposes it the way the GitHub provider exposes its own. Co-Authored-By: Claude Opus 5.5 --- .../server/GitLabPullRequestProvider.test.ts | 66 +++++++++++++++++++ .../src/server/GitLabPullRequestProvider.ts | 3 + 2 files changed, 69 insertions(+) diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts index d506aace9489..6800bc467541 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts @@ -212,3 +212,69 @@ describe("rewriting what has already been said", () => { }), ); }); + +describe("expanding unchanged lines", () => { + const change = { + cwd: "/w", + repository: "acme/web", + host: "gitlab.com", + number: 7, + changeType: "change" as const, + oldPath: "src/page.ts", + newPath: "src/page.ts", + }; + + it.effect("reads both sides of a file through the merge request or one of its commits", () => + Effect.gen(function* () { + const getMergeRequestDiffFileContents = vi.fn(() => + Effect.succeed({ oldContents: "before\n", newContents: "after\n" }), + ); + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(GitLabPullRequestCli.GitLabPullRequestCli)({ + getMergeRequestDiffFileContents, + }), + ), + ); + assert.isDefined(provider.getDiffFileContents); + const oneCommit = { ...change, commit: "a1b2c3d" }; + + expect(yield* provider.getDiffFileContents(change)).toEqual({ + oldContents: "before\n", + newContents: "after\n", + }); + yield* provider.getDiffFileContents(oneCommit); + expect(getMergeRequestDiffFileContents).toHaveBeenNthCalledWith(1, change); + expect(getMergeRequestDiffFileContents).toHaveBeenNthCalledWith(2, oneCommit); + }), + ); + + it.effect("reports a file GitLab cannot expand as a failed read", () => + Effect.gen(function* () { + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(GitLabPullRequestCli.GitLabPullRequestCli)({ + getMergeRequestDiffFileContents: () => + Effect.fail( + new GitLabPullRequestCli.GitLabDiffFileContentsUnavailableError({ + command: "glab", + cwd: "/w", + path: "assets/logo.png", + reason: "binary", + }), + ), + }), + ), + ); + assert.isDefined(provider.getDiffFileContents); + + expect(yield* Effect.flip(provider.getDiffFileContents(change))).toMatchObject({ + _tag: "PullRequestProviderError", + provider: "gitlab", + operation: "getDiffFileContents", + reason: "failed", + detail: "The diff file 'assets/logo.png' is binary.", + }); + }), + ); +}); diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts index 1b5b987cd5ac..d9d04b02e554 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts @@ -230,6 +230,9 @@ export const make = Effect.gen(function* () { getDiff: (input) => cli.getMergeRequestDiff(input).pipe(Effect.mapError(fail("getDiff"))), + getDiffFileContents: (input) => + cli.getMergeRequestDiffFileContents(input).pipe(Effect.mapError(fail("getDiffFileContents"))), + // What each marked file is at the head, which is what tells a mark that still stands from one // the branch has moved past. GitLab's own local-storage marks are keyed on the blob id too, // so this stales at the same moment its web UI would.