feat(web): resolve walkthrough diffs for GitLab merge requests
This commit is contained in:
@@ -20,7 +20,7 @@ has to ask for it.
|
|||||||
`PROMPT_VERSION`.
|
`PROMPT_VERSION`.
|
||||||
- `schema.js` — response schema, response normalization, tolerant JSON parsing.
|
- `schema.js` — response schema, response normalization, tolerant JSON parsing.
|
||||||
- `store.js` — content-addressed cache entries plus mutable pointers.
|
- `store.js` — content-addressed cache entries plus mutable pointers.
|
||||||
- `pull-request.js` — PR diffs via the shared GitHub octokit helper.
|
- `pull-request.js` — PR/MR diffs; the provider (GitHub octokit or GitLab REST client) is chosen by the git remote.
|
||||||
- `model-settings.js` — the feature's own model override.
|
- `model-settings.js` — the feature's own model override.
|
||||||
- `languages.js` — the languages the prose may be written in.
|
- `languages.js` — the languages the prose may be written in.
|
||||||
- `index.js` — orchestration.
|
- `index.js` — orchestration.
|
||||||
@@ -53,7 +53,15 @@ written against staged code never silently re-anchors onto an unstaged edit.
|
|||||||
|---|---|---|
|
|---|---|---|
|
||||||
| `working-tree` (`all` \| `staged` \| `working`) | `staged`, `working` | Untracked files are fetched individually because `git diff` omits them |
|
| `working-tree` (`all` \| `staged` \| `working`) | `staged`, `working` | Untracked files are fetched individually because `git diff` omits them |
|
||||||
| `branch` | `branch` | `getRangeDiff` uses three-dot `base...head`, so work merged in from the base branch is excluded |
|
| `branch` | `branch` | `getRangeDiff` uses three-dot `base...head`, so work merged in from the base branch is excluded |
|
||||||
| `pr` | `pr:<number>` | GitHub returns the merge-base diff, matching the branch semantics |
|
| `pr` | `pr:<number>` | GitHub returns the merge-base diff; a GitLab remote concatenates the merge request's diffs instead — both match the branch semantics |
|
||||||
|
|
||||||
|
Provider selection lives in `pull-request.js`: the directory's git remote
|
||||||
|
decides. A GitLab remote resolves through `resolveGitLabRepoFromDirectory` and
|
||||||
|
fetches `merge_requests/:iid/diffs` pages (capped at 10), concatenating the
|
||||||
|
per-file diffs into one patch; anything else falls back to the GitHub pull
|
||||||
|
request API. A GitLab directory without a connected GitLab account fails with
|
||||||
|
`401 gitlab-not-connected`, and an MR with no diffs fails with
|
||||||
|
`404 empty-diff`, the same code an empty GitHub PR uses.
|
||||||
|
|
||||||
For the current-branch source, the UI takes the base from the default branch of
|
For the current-branch source, the UI takes the base from the default branch of
|
||||||
the current branch's tracking remote (`defaultBranches` in the branches
|
the current branch's tracking remote (`defaultBranches` in the branches
|
||||||
|
|||||||
@@ -1,14 +1,20 @@
|
|||||||
import { getOctokitOrNull } from '../github/octokit.js';
|
import { getOctokitOrNull } from '../github/octokit.js';
|
||||||
import { resolveGitHubRepoFromDirectory } from '../github/repo/index.js';
|
import { resolveGitHubRepoFromDirectory } from '../github/repo/index.js';
|
||||||
|
import { getGitLabClientOrNull } from '../gitlab/client.js';
|
||||||
|
import { resolveGitLabRepoFromDirectory } from '../gitlab/repo.js';
|
||||||
|
|
||||||
|
// GitLab diff pagination cap: never loop more than 10 pages of 100 files,
|
||||||
|
// mirroring gitlab/routes.js.
|
||||||
|
const GITLAB_DIFFS_MAX_PAGES = 10;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Raw unified diff for a pull request.
|
* Raw unified diff for a GitHub pull request.
|
||||||
*
|
*
|
||||||
* GitHub already returns the merge-base diff for a PR, so this matches the
|
* GitHub already returns the merge-base diff for a PR, so this matches the
|
||||||
* three-dot semantics used for local branch reviews: work merged in from the
|
* three-dot semantics used for local branch reviews: work merged in from the
|
||||||
* base branch is not part of it.
|
* base branch is not part of it.
|
||||||
*/
|
*/
|
||||||
export async function getPullRequestDiff(directory, number) {
|
async function getGitHubPullRequestDiff(directory, number) {
|
||||||
const octokit = getOctokitOrNull();
|
const octokit = getOctokitOrNull();
|
||||||
if (!octokit) {
|
if (!octokit) {
|
||||||
throw Object.assign(new Error('Connect a GitHub account to review pull requests'), {
|
throw Object.assign(new Error('Connect a GitHub account to review pull requests'), {
|
||||||
@@ -44,3 +50,75 @@ export async function getPullRequestDiff(directory, number) {
|
|||||||
|
|
||||||
return { patch, meta: { owner: repo.owner, repo: repo.repo, number } };
|
return { patch, meta: { owner: repo.owner, repo: repo.repo, number } };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Raw unified diff for a GitLab merge request.
|
||||||
|
*
|
||||||
|
* GitLab's merge request diffs endpoint returns one entry per file, so the
|
||||||
|
* pages are concatenated into a single patch. `repo` comes from the
|
||||||
|
* dispatcher's `resolveGitLabRepoFromDirectory` call and is never re-resolved
|
||||||
|
* here.
|
||||||
|
*/
|
||||||
|
async function getGitLabMergeRequestDiff(repo, number) {
|
||||||
|
const client = getGitLabClientOrNull();
|
||||||
|
if (!client) {
|
||||||
|
throw Object.assign(new Error('Connect a GitLab account to review merge requests'), {
|
||||||
|
statusCode: 401,
|
||||||
|
code: 'gitlab-not-connected',
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
// The parser always populates both fields; this guards a malformed repo so
|
||||||
|
// the failure is explicit rather than a downstream TypeError.
|
||||||
|
if (!repo?.namespace || !repo?.project) {
|
||||||
|
throw Object.assign(new Error('This directory has no GitLab remote'), {
|
||||||
|
statusCode: 400,
|
||||||
|
code: 'no-gitlab-remote',
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
// The client URL-encodes the path internally; never pre-encode it.
|
||||||
|
const projectPath = `${repo.namespace}/${repo.project}`;
|
||||||
|
|
||||||
|
const diffs = [];
|
||||||
|
for (let page = 1; page <= GITLAB_DIFFS_MAX_PAGES; page += 1) {
|
||||||
|
const response = await client.mergeRequestDiffs(projectPath, number, { per_page: 100, page });
|
||||||
|
if (response.status !== 200 || !Array.isArray(response.data)) {
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
diffs.push(...response.data);
|
||||||
|
// The page object is the authoritative signal; the 10-page cap above is
|
||||||
|
// what stops a server that lies about hasMore from looping forever.
|
||||||
|
if (!response.page?.hasMore) {
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
const patch = diffs
|
||||||
|
.map((item) => (typeof item?.diff === 'string' ? item.diff : ''))
|
||||||
|
.filter(Boolean)
|
||||||
|
.join('\n');
|
||||||
|
if (!patch.trim()) {
|
||||||
|
throw Object.assign(new Error(`Merge request #${number} has no diff`), {
|
||||||
|
statusCode: 404,
|
||||||
|
code: 'empty-diff',
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
return { patch, meta: { namespace: repo.namespace, project: repo.project, number } };
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Raw unified diff for a pull request or merge request.
|
||||||
|
*
|
||||||
|
* The provider is chosen by the repository's git remote: a GitLab remote uses
|
||||||
|
* the GitLab merge request API, anything else falls back to the GitHub pull
|
||||||
|
* request API.
|
||||||
|
*/
|
||||||
|
export async function getPullRequestDiff(directory, number) {
|
||||||
|
const { repo } = await resolveGitLabRepoFromDirectory(directory);
|
||||||
|
if (repo) {
|
||||||
|
return getGitLabMergeRequestDiff(repo, number);
|
||||||
|
}
|
||||||
|
return getGitHubPullRequestDiff(directory, number);
|
||||||
|
}
|
||||||
|
|||||||
@@ -2,10 +2,14 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
|||||||
|
|
||||||
vi.mock('../github/octokit.js', () => ({ getOctokitOrNull: vi.fn() }));
|
vi.mock('../github/octokit.js', () => ({ getOctokitOrNull: vi.fn() }));
|
||||||
vi.mock('../github/repo/index.js', () => ({ resolveGitHubRepoFromDirectory: vi.fn() }));
|
vi.mock('../github/repo/index.js', () => ({ resolveGitHubRepoFromDirectory: vi.fn() }));
|
||||||
|
vi.mock('../gitlab/client.js', () => ({ getGitLabClientOrNull: vi.fn() }));
|
||||||
|
vi.mock('../gitlab/repo.js', () => ({ resolveGitLabRepoFromDirectory: vi.fn() }));
|
||||||
|
|
||||||
const { getPullRequestDiff } = await import('./pull-request.js');
|
const { getPullRequestDiff } = await import('./pull-request.js');
|
||||||
const { getOctokitOrNull } = await import('../github/octokit.js');
|
const { getOctokitOrNull } = await import('../github/octokit.js');
|
||||||
const { resolveGitHubRepoFromDirectory } = await import('../github/repo/index.js');
|
const { resolveGitHubRepoFromDirectory } = await import('../github/repo/index.js');
|
||||||
|
const { getGitLabClientOrNull } = await import('../gitlab/client.js');
|
||||||
|
const { resolveGitLabRepoFromDirectory } = await import('../gitlab/repo.js');
|
||||||
|
|
||||||
const PATCH = `diff --git a/src/a.ts b/src/a.ts
|
const PATCH = `diff --git a/src/a.ts b/src/a.ts
|
||||||
--- a/src/a.ts
|
--- a/src/a.ts
|
||||||
@@ -14,6 +18,28 @@ const PATCH = `diff --git a/src/a.ts b/src/a.ts
|
|||||||
+const added = true;
|
+const added = true;
|
||||||
`;
|
`;
|
||||||
|
|
||||||
|
const GITLAB_DIFF_ONE = `diff --git a/a.txt b/a.txt
|
||||||
|
--- a/a.txt
|
||||||
|
+++ b/a.txt
|
||||||
|
@@ -1,1 +1,2 @@
|
||||||
|
+hello
|
||||||
|
`;
|
||||||
|
|
||||||
|
const GITLAB_DIFF_TWO = `diff --git a/b.txt b/b.txt
|
||||||
|
--- a/b.txt
|
||||||
|
+++ b/b.txt
|
||||||
|
@@ -1 +1,2 @@
|
||||||
|
+world
|
||||||
|
`;
|
||||||
|
|
||||||
|
const GITLAB_REPO = {
|
||||||
|
namespace: 'acme',
|
||||||
|
project: 'widgets',
|
||||||
|
host: 'gitlab.com',
|
||||||
|
baseUrl: 'https://gitlab.com',
|
||||||
|
url: 'https://gitlab.com/acme/widgets',
|
||||||
|
};
|
||||||
|
|
||||||
describe('getPullRequestDiff', () => {
|
describe('getPullRequestDiff', () => {
|
||||||
let request;
|
let request;
|
||||||
|
|
||||||
@@ -27,6 +53,10 @@ describe('getPullRequestDiff', () => {
|
|||||||
repo: { owner: 'openchamber', repo: 'openchamber' },
|
repo: { owner: 'openchamber', repo: 'openchamber' },
|
||||||
remoteUrl: 'git@github.com:openchamber/openchamber.git',
|
remoteUrl: 'git@github.com:openchamber/openchamber.git',
|
||||||
});
|
});
|
||||||
|
// Default to a non-GitLab directory so the GitHub cases keep routing to
|
||||||
|
// the GitHub path.
|
||||||
|
resolveGitLabRepoFromDirectory.mockResolvedValue({ repo: null, remoteUrl: null });
|
||||||
|
getGitLabClientOrNull.mockReturnValue(null);
|
||||||
});
|
});
|
||||||
|
|
||||||
afterEach(() => {
|
afterEach(() => {
|
||||||
@@ -74,4 +104,81 @@ describe('getPullRequestDiff', () => {
|
|||||||
statusCode: 404,
|
statusCode: 404,
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('with a GitLab repository', () => {
|
||||||
|
let mergeRequestDiffs;
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
mergeRequestDiffs = vi.fn().mockResolvedValue({
|
||||||
|
status: 200,
|
||||||
|
data: [{ diff: GITLAB_DIFF_ONE }],
|
||||||
|
page: { page: 1, next: null, total: 1, hasMore: false },
|
||||||
|
});
|
||||||
|
getGitLabClientOrNull.mockReturnValue({ mergeRequestDiffs });
|
||||||
|
resolveGitLabRepoFromDirectory.mockResolvedValue({
|
||||||
|
repo: GITLAB_REPO,
|
||||||
|
remoteUrl: 'git@gitlab.com:acme/widgets.git',
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it('concatenates merge request diffs across pages into a single patch', async () => {
|
||||||
|
mergeRequestDiffs
|
||||||
|
.mockResolvedValueOnce({
|
||||||
|
status: 200,
|
||||||
|
data: [{ diff: GITLAB_DIFF_ONE }],
|
||||||
|
page: { page: 1, next: 2, total: 2, hasMore: true },
|
||||||
|
})
|
||||||
|
.mockResolvedValueOnce({
|
||||||
|
status: 200,
|
||||||
|
data: [{ diff: GITLAB_DIFF_TWO }],
|
||||||
|
page: { page: 2, next: null, total: 2, hasMore: false },
|
||||||
|
});
|
||||||
|
|
||||||
|
const result = await getPullRequestDiff('/repo', 7);
|
||||||
|
|
||||||
|
expect(result.patch).toBe(`${GITLAB_DIFF_ONE}\n${GITLAB_DIFF_TWO}`);
|
||||||
|
expect(result.meta).toEqual({ namespace: 'acme', project: 'widgets', number: 7 });
|
||||||
|
// The unencoded namespace/project path is passed to the client, which
|
||||||
|
// URL-encodes it internally.
|
||||||
|
expect(mergeRequestDiffs).toHaveBeenCalledTimes(2);
|
||||||
|
expect(mergeRequestDiffs).toHaveBeenNthCalledWith(1, 'acme/widgets', 7, { per_page: 100, page: 1 });
|
||||||
|
expect(mergeRequestDiffs).toHaveBeenNthCalledWith(2, 'acme/widgets', 7, { per_page: 100, page: 2 });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('asks the user to connect GitLab before fetching diffs', async () => {
|
||||||
|
getGitLabClientOrNull.mockReturnValue(null);
|
||||||
|
|
||||||
|
await expect(getPullRequestDiff('/repo', 7)).rejects.toMatchObject({
|
||||||
|
code: 'gitlab-not-connected',
|
||||||
|
statusCode: 401,
|
||||||
|
});
|
||||||
|
expect(mergeRequestDiffs).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('treats an MR with no diff as missing rather than an empty review', async () => {
|
||||||
|
mergeRequestDiffs.mockResolvedValue({
|
||||||
|
status: 200,
|
||||||
|
data: [{ diff: ' ' }],
|
||||||
|
page: { page: 1, next: null, total: 1, hasMore: false },
|
||||||
|
});
|
||||||
|
|
||||||
|
await expect(getPullRequestDiff('/repo', 7)).rejects.toMatchObject({
|
||||||
|
code: 'empty-diff',
|
||||||
|
statusCode: 404,
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it('reports a GitLab repo without namespace or project as having no remote', async () => {
|
||||||
|
resolveGitLabRepoFromDirectory.mockResolvedValue({
|
||||||
|
repo: { namespace: '', project: '' },
|
||||||
|
remoteUrl: 'git@gitlab.com:acme/widgets.git',
|
||||||
|
});
|
||||||
|
|
||||||
|
await expect(getPullRequestDiff('/repo', 7)).rejects.toMatchObject({
|
||||||
|
code: 'no-gitlab-remote',
|
||||||
|
statusCode: 400,
|
||||||
|
});
|
||||||
|
expect(mergeRequestDiffs).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user