提交 f4e6e869 编写于 作者: T Tomas Vik

feat: create new MR diff comments

上级 69f52033
......@@ -166,9 +166,9 @@ You can use [GitLab Slash Commands](https://docs.gitlab.com/ee/integration/slash
#### Merge Request Reviews
GitLab Workflow allows you to review changes and comments on merge requests directly inside the editor. You can navigate to merge requests on the left hand sidebar and expand any relevant merge requests to see both the description and files changed.
GitLab Workflow allows you to review merge requests directly inside the editor. You can navigate to merge requests on the left hand sidebar and expand any relevant merge request to see both the description and files changed.
When navigating changed files you can review discussions that have been made on the diff. You can resolve and unresolve these discussions and also delete and edit individual comments.
When navigating changed files you can review and create discussions on the diff. You can resolve and unresolve these discussions and also delete and edit individual comments.
![_diff-comments_.gif](https://gitlab.com/gitlab-org/gitlab-vscode-extension/raw/main/src/assets/_diff-comments.gif)
......
import * as vscode from 'vscode';
import { mocked } from 'ts-jest/utils';
import {
discussionOnDiff,
noteOnDiff,
} from '../../test/integration/fixtures/graphql/discussions.js';
import { gitExtensionWrapper } from '../git/git_extension_wrapper';
import { GqlTextDiffNote } from '../gitlab/graphql/shared';
import { GitLabComment } from '../review/gitlab_comment';
import { deleteComment } from './mr_discussion_commands';
import { GitLabCommentThread } from '../review/gitlab_comment_thread';
import { toReviewUri } from '../review/review_uri';
import { deleteComment, createComment } from './mr_discussion_commands';
import { WrappedRepository } from '../git/wrapped_repository';
import { mr, mrVersion } from '../test_utils/entities';
jest.mock('../git/git_extension_wrapper');
describe('MR discussion commands', () => {
describe('deleteComment', () => {
......@@ -39,4 +52,118 @@ describe('MR discussion commands', () => {
expect(mockedComment.thread.deleteComment).not.toHaveBeenCalled();
});
});
describe('create comment', () => {
describe('responding in a thread', () => {
it('responds if the thread already contains comments', async () => {
const mockedGitLabThread = ({
reply: jest.fn(),
} as unknown) as GitLabCommentThread;
const comment = GitLabComment.fromGqlNote(
noteOnDiff as GqlTextDiffNote,
mockedGitLabThread,
);
const mockedVsThread = ({
comments: [comment],
} as unknown) as vscode.CommentThread;
await createComment({
text: 'reply text',
thread: mockedVsThread,
});
expect(comment.thread.reply).toHaveBeenCalledWith('reply text');
});
});
describe('creating a new thread', () => {
// this diff hunk represents the diff where we create a new comment thread
const diff = [
'@@ -1,10 +1,10 @@',
' 1',
'-2', // line 2 exists only on the old version of the diff
'-3',
' 4', // unchanged - line #4 on the old version and #2 on the new version
' 5',
' 6',
' 7',
' 8',
'+8.1', // this line exists only as line #7 on the new version of the diff
'+8.2',
' 9',
' 10',
].join('\n');
const mrDiff = {
...mrVersion.diffs[0],
diff,
old_path: 'old/path/to/file.js',
new_path: 'new/path/to/file.js',
};
const customMrVersion = {
...mrVersion,
base_commit_sha: 'aaaaa',
head_commit_sha: 'bbbbb',
start_commit_sha: 'ccccc',
diffs: [mrDiff],
};
const createVsThread = (filePath: string, fileCommit: string, lineNumber: number) => {
const uri = toReviewUri({
path: filePath,
commit: fileCommit,
repositoryRoot: 'root',
projectId: mr.project_id,
mrId: mr.id,
});
return ({
comments: [],
uri,
range: {
start: new vscode.Position(lineNumber - 1, 0), // VS Code indexes lines starting from 0
},
} as unknown) as vscode.CommentThread;
};
it.each`
scenario | filePath | fileCommit | lineNumber | expectedOldLine | expectedNewLine
${'old line'} | ${mrDiff.old_path} | ${customMrVersion.base_commit_sha} | ${2} | ${2} | ${undefined}
${'new line'} | ${mrDiff.new_path} | ${customMrVersion.head_commit_sha} | ${7} | ${undefined} | ${7}
${'unchanged line'} | ${mrDiff.old_path} | ${customMrVersion.base_commit_sha} | ${4} | ${4} | ${2}
`(
'creates thread correctly for $scenario',
async ({ filePath, fileCommit, lineNumber, expectedOldLine, expectedNewLine }) => {
const mockedCreateDiffNote = jest.fn().mockResolvedValue(discussionOnDiff);
const mockedWrappedRepository = {
getMr: () => ({ mr, mrVersion: customMrVersion }),
getGitLabService: () => ({
createDiffNote: mockedCreateDiffNote,
}),
};
mocked(gitExtensionWrapper).getRepository.mockReturnValue(
(mockedWrappedRepository as unknown) as WrappedRepository,
);
await createComment({
text: 'new thread text',
thread: createVsThread(filePath, fileCommit, lineNumber),
});
expect(mockedCreateDiffNote).toHaveBeenCalledWith(mr.id, 'new thread text', {
baseSha: 'aaaaa',
headSha: 'bbbbb',
startSha: 'ccccc',
paths: {
oldPath: 'old/path/to/file.js',
newPath: 'new/path/to/file.js',
},
oldLine: expectedOldLine,
newLine: expectedNewLine,
});
},
);
});
});
});
import * as assert from 'assert';
import * as vscode from 'vscode';
import { getNewLineForOldUnchangedLine } from '../git/diff_line_count';
import { gitExtensionWrapper } from '../git/git_extension_wrapper';
import { GitLabComment } from '../review/gitlab_comment';
import { GitLabCommentThread } from '../review/gitlab_comment_thread';
import { fromReviewUri } from '../review/review_uri';
import { findFileInDiffs } from '../utils/find_file_in_diffs';
const getGitLabThreadFromVsThread = (thread: vscode.CommentThread): GitLabCommentThread => {
const firstComment = thread.comments[0];
assert(firstComment && firstComment instanceof GitLabComment);
return firstComment.thread;
const getLineNumber = (thread: vscode.CommentThread) => thread.range.start.line + 1;
const createNewComment = async (
text: string,
thread: vscode.CommentThread,
): Promise<GitLabCommentThread> => {
const { path, commit, repositoryRoot, mrId } = fromReviewUri(thread.uri);
const repository = gitExtensionWrapper.getRepository(repositoryRoot);
const cachedMr = repository.getMr(mrId);
assert(cachedMr);
const { mr, mrVersion } = cachedMr;
const isOld = commit === mrVersion.base_commit_sha;
const diff = findFileInDiffs(mrVersion.diffs, isOld ? { oldPath: path } : { newPath: path });
assert(diff);
const positionFragment = isOld
? {
oldLine: getLineNumber(thread),
// we let user comment on any line on the old version of the diff
// this means some of the lines might be unchanged
// till https://gitlab.com/gitlab-org/gitlab/-/issues/325161 gets fixed, we need to compute
// the new line index for unchanged line.
newLine: getNewLineForOldUnchangedLine(mrVersion, path!, getLineNumber(thread)),
}
: { newLine: getLineNumber(thread) };
const discussion = await repository.getGitLabService().createDiffNote(mrId, text, {
baseSha: mrVersion.base_commit_sha,
headSha: mrVersion.head_commit_sha,
startSha: mrVersion.start_commit_sha,
paths: {
oldPath: diff.old_path,
newPath: diff.new_path,
},
...positionFragment,
});
return new GitLabCommentThread(thread, discussion, repository.getGitLabService(), mr);
};
export const toggleResolved = async (vsThread: vscode.CommentThread): Promise<void> => {
const gitlabThread = getGitLabThreadFromVsThread(vsThread);
const firstComment = vsThread.comments[0];
assert(firstComment instanceof GitLabComment);
const gitlabThread = firstComment.thread;
return gitlabThread.toggleResolved();
};
......@@ -46,6 +85,13 @@ export const createComment = async ({
text: string;
thread: vscode.CommentThread;
}): Promise<void> => {
const gitlabThread = getGitLabThreadFromVsThread(thread);
const firstComment = thread.comments[0];
if (!firstComment) {
await createNewComment(text, thread);
return undefined;
}
assert(firstComment instanceof GitLabComment);
const gitlabThread = firstComment.thread;
return gitlabThread.reply(text);
};
......@@ -45,7 +45,7 @@ describe('MrItemModel', () => {
await item.getChildren();
expect(createCommentControllerMock).toBeCalledWith('gitlab-org/gitlab!2000', 'Issuable Title');
const [uri, range] = createCommentThreadMock.mock.calls[0];
expect(uri.path).toBe('src/webview/src/components/LabelNote.vue');
expect(uri.path).toBe('src/webview/src/components/LabelNoteOld.vue');
expect(range.start.line).toBe(47);
expect(commentThread.comments.length).toBe(1);
const firstComment = commentThread.comments[0];
......
......@@ -4,8 +4,11 @@ import { DiffFilePath, findFileInDiffs } from '../utils/find_file_in_diffs';
// these helper functions are simplified version of the same lodash functions
const range = (start: number, end: number) => [...Array(end - start).keys()].map(n => n + start);
const flatten = <T>(a: T[][]): T[] => a.reduce((acc, nested) => [...acc, ...nested], []);
const last = <T>(a: T[]): T => a[a.length - 1];
const first = <T>(a: T[]): T => a[0];
const last = <T>(a: T[]): T | undefined => a[a.length - 1];
const first = <T>(a: T[]): T | undefined => a[0];
// copied from https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_isempty
const isEmpty = (obj: any): boolean =>
[Object, Array].includes((obj || {}).constructor) && !Object.entries(obj || {}).length;
/**
* This method returns line number where in the text document given hunk starts.
......@@ -35,7 +38,7 @@ const UNCHANGED = 'UNCHANGED';
type RemovedLine = { type: typeof REMOVED; oldLine: number };
type AddedLine = { type: typeof ADDED; newLine: number };
type UnchangedLine = { type: typeof UNCHANGED; oldLine: number; newLine: number };
export type UnchangedLine = { type: typeof UNCHANGED; oldLine: number; newLine: number };
type HunkLine = RemovedLine | AddedLine | UnchangedLine;
/** Converts lines in the text hunk into data structures that represent type of the change and affected lines */
......@@ -111,7 +114,7 @@ const connectHunks = (parsedHunks: HunkLine[][]): HunkLine[] =>
? []
: parsedHunks.reduce((acc, hunk) => [
...acc,
...createUnchangedLinesBetweenHunks(last(acc), first(hunk)),
...createUnchangedLinesBetweenHunks(last(acc)!, first(hunk)!),
...hunk,
]);
......@@ -119,3 +122,17 @@ export const getUnchangedLines = (mrVersion: RestMrVersion, oldPath: string): Un
connectHunks(getHunksForFile(mrVersion, { oldPath })).filter(
(l): l is UnchangedLine => l.type === UNCHANGED,
);
export const getNewLineForOldUnchangedLine = (
mrVersion: RestMrVersion,
oldPath: string,
oldLine: number,
): number | undefined => {
const unchangedLines = getUnchangedLines(mrVersion, oldPath);
if (isEmpty(unchangedLines)) return undefined;
if (oldLine < first(unchangedLines)!.oldLine)
return oldLine + newLineOffset(first(unchangedLines)!);
if (oldLine > last(unchangedLines)!.oldLine)
return oldLine + newLineOffset(last(unchangedLines)!);
return unchangedLines.find(ul => ul.oldLine === oldLine)?.newLine;
};
......@@ -21,6 +21,7 @@ import {
GetDiscussionsQueryOptions,
GqlDiscussion,
GetDiscussionsQueryResult,
GqlTextDiffDiscussion,
} from './graphql/get_discussions';
import {
GetSnippetsQueryOptions,
......@@ -34,6 +35,7 @@ import {
GetProjectQueryResult,
queryGetProject,
} from './graphql/get_project';
import { createDiffNoteMutation, GqlDiffPositionInput } from './graphql/create_diff_comment';
import { removeLeadingSlash } from '../utils/remove_leading_slash';
interface CreateNoteResult {
......@@ -109,6 +111,7 @@ const getProjectPath = (issuable: RestIssuable) => issuable.references.full.spli
const isMr = (issuable: RestIssuable) => Boolean(issuable.sha);
const getIssuableGqlId = (issuable: RestIssuable) =>
`gid://gitlab/${isMr(issuable) ? 'MergeRequest' : 'Issue'}/${issuable.id}`;
const getMrGqlId = (id: number) => `gid://gitlab/MergeRequest/${id}`;
export class GitLabNewService {
client: GraphQLClient;
......@@ -398,4 +401,29 @@ export class GitLabNewService {
);
}
}
async createDiffNote(
mrId: number,
body: string,
position: GqlDiffPositionInput,
): Promise<GqlTextDiffDiscussion> {
try {
const result = await this.client.request(createDiffNoteMutation, {
issuableId: getMrGqlId(mrId),
body,
position,
});
assert(
result?.createDiffNote?.note?.discussion,
`Response doesn't contain a note with discussion: ${JSON.stringify(result)}`,
);
return result.createDiffNote.note.discussion;
} catch (e) {
throw new UserFriendlyError(
`Extension failed to create the comment.
Open extension logs to see your comment text and find more details about the error.`,
new Error(`MR(${mrId}), text(${body}),\n${JSON.stringify(position)}, ${e}`),
);
}
}
}
import { gql } from 'graphql-request';
import { discussionDetailsFragment } from './shared';
export interface GqlDiffPositionInput {
baseSha: string;
headSha: string;
startSha: string;
paths: {
newPath: string;
oldPath: string;
};
newLine?: number;
oldLine?: number;
}
export const createDiffNoteMutation = gql`
${discussionDetailsFragment}
mutation CreateDiffNote($issuableId: NoteableID!, $body: String!, $position: DiffPositionInput!) {
createDiffNote(input: { noteableId: $issuableId, body: $body, position: $position }) {
errors
note {
discussion {
...discussionDetails
}
}
}
}
`;
import { gql } from 'graphql-request';
import {
noteDetailsFragment,
discussionDetailsFragment,
Node,
GqlProjectResult,
GqlNote,
......@@ -10,26 +10,14 @@ import {
} from './shared';
const discussionsFragment = gql`
${noteDetailsFragment}
${discussionDetailsFragment}
fragment discussions on DiscussionConnection {
pageInfo {
hasNextPage
endCursor
}
nodes {
replyId
createdAt
resolved
resolvable
notes {
pageInfo {
hasNextPage
endCursor
}
nodes {
...noteDetails
}
}
...discussionDetails
}
}
`;
......
......@@ -57,6 +57,25 @@ export const noteDetailsFragment = gql`
}
`;
export const discussionDetailsFragment = gql`
${noteDetailsFragment}
fragment discussionDetails on Discussion {
replyId
createdAt
resolved
resolvable
notes {
pageInfo {
hasNextPage
endCursor
}
nodes {
...noteDetails
}
}
}
`;
interface GqlGroup {
id: string;
}
......
......@@ -6,9 +6,12 @@ import {
} from '../../test/integration/fixtures/graphql/discussions.js';
import { GitLabComment } from './gitlab_comment';
import { GitLabNewService } from '../gitlab/gitlab_new_service';
import { mr } from '../test_utils/entities';
import { GqlTextDiffNote } from '../gitlab/graphql/shared';
import { mr, project } from '../test_utils/entities';
import { GqlTextDiffNote, GqlTextPosition } from '../gitlab/graphql/shared';
import { GqlTextDiffDiscussion } from '../gitlab/graphql/get_discussions';
import { fromReviewUri } from './review_uri';
const TEST_ROOT = '/repositoryRoot';
describe('GitLabCommentThread', () => {
let gitlabCommentThread: GitLabCommentThread;
......@@ -65,7 +68,7 @@ describe('GitLabCommentThread', () => {
} as unknown) as GitLabNewService;
gitlabCommentThread = GitLabCommentThread.createThread({
commentController: fakeCommentController,
repositoryRoot: '/repositoryRoot',
repositoryRoot: TEST_ROOT,
mr,
discussion,
gitlabService,
......@@ -84,6 +87,54 @@ describe('GitLabCommentThread', () => {
expect(vsCommentThread.range.start.line).toBe(noteOnDiff.position.oldLine - 1); // vs code numbers lines from 0
});
describe('new thread URI', () => {
it('populates basic uri parameters', () => {
const { projectId, repositoryRoot, mrId } = fromReviewUri(vsCommentThread.uri);
expect(projectId).toBe(mr.project_id);
expect(mrId).toBe(mr.id);
expect(repositoryRoot).toBe(TEST_ROOT);
});
it('generates uri for discussion on old diff version', () => {
createGitLabCommentThread(
createGqlTextDiffDiscussion({
...noteOnDiff,
position: {
...noteOnDiff.position,
oldLine: 10, // comment is on the old version of the diff
newLine: null,
oldPath: 'old/path.js',
} as GqlTextPosition,
}),
);
const { commit, path } = fromReviewUri(vsCommentThread.uri);
expect(commit).toBe(noteOnDiff.position.diffRefs.baseSha);
expect(path).toBe('old/path.js');
});
it('generates uri for discussion on new diff version', () => {
createGitLabCommentThread(
createGqlTextDiffDiscussion({
...noteOnDiff,
position: {
...noteOnDiff.position,
oldLine: null,
newLine: 10, // comment is on the new version of the diff
newPath: 'new/path.js',
} as GqlTextPosition,
}),
);
const { commit, path } = fromReviewUri(vsCommentThread.uri);
expect(commit).toBe(noteOnDiff.position.diffRefs.headSha);
expect(path).toBe('new/path.js');
});
});
it('generates uri for the discussion', () => {
// TODO: improve the uri tests once we merge https://gitlab.com/gitlab-org/gitlab-vscode-extension/-/merge_requests/192
const { baseSha } = noteOnDiff.position.diffRefs; // baseSha points to the commit that the MR started from (old lines)
......
......@@ -22,22 +22,11 @@ const commentRangeFromPosition = (position: GqlTextPosition): vscode.Range => {
return new vscode.Range(vsPosition, vsPosition);
};
const uriFromPosition = (
position: GqlTextPosition,
repositoryRoot: string,
gitlabProjectId: number,
mrId: number,
) => {
const pathAndCommitFromPosition = (position: GqlTextPosition) => {
const onOldVersion = position.oldLine !== null;
const path = onOldVersion ? position.oldPath : position.newPath;
const commit = onOldVersion ? position.diffRefs.baseSha : position.diffRefs.headSha;
return toReviewUri({
path,
commit,
repositoryRoot,
projectId: gitlabProjectId,
mrId,
});
return { path, commit };
};
interface CreateThreadOptions {
......@@ -51,12 +40,17 @@ interface CreateThreadOptions {
export class GitLabCommentThread {
private resolved: boolean;
private constructor(
/** Has a side-effect of populating the vsThread with all comments */
constructor(
private vsThread: vscode.CommentThread,
private gqlDiscussion: GqlTextDiffDiscussion,
private gitlabService: GitLabNewService,
private mr: RestIssuable,
) {
// SIDE-EFFECT
this.vsThread.comments = gqlDiscussion.notes.nodes.map(note =>
GitLabComment.fromGqlNote(note, this),
);
this.vsThread.collapsibleState = vscode.CommentThreadCollapsibleState.Expanded;
this.vsThread.canReply = firstNoteFrom(gqlDiscussion).userPermissions.createNote;
this.resolved = gqlDiscussion.resolved;
......@@ -146,16 +140,17 @@ export class GitLabCommentThread {
}: CreateThreadOptions): GitLabCommentThread {
const { position } = firstNoteFrom(discussion);
const vsThread = commentController.createCommentThread(
uriFromPosition(position, repositoryRoot, mr.project_id, mr.id),
toReviewUri({
...pathAndCommitFromPosition(position),
repositoryRoot,
projectId: mr.project_id,
mrId: mr.id,
}),
commentRangeFromPosition(position),
// the comments need to know about the thread, so we first
// create empty thread to be able to create comments
[],
);
const glThread = new GitLabCommentThread(vsThread, discussion, gitlabService, mr);
vsThread.comments = discussion.notes.nodes.map(note =>
GitLabComment.fromGqlNote(note, glThread),
);
return glThread;
return new GitLabCommentThread(vsThread, discussion, gitlabService, mr);
}
}
......@@ -109,12 +109,12 @@ const noteOnDiff = {
headSha: 'b9c6f9ad70d55a75785fb2702ab8012a69e767d3',
startSha: 'b9c6f9ad70d55a75785fb2702ab8012a69e767d3',
},
filePath: 'src/webview/src/components/LabelNote.vue',
filePath: 'src/webview/src/components/LabelNoteOld.vue',
positionType: 'text',
newLine: null,
oldLine: 48,
newPath: 'src/webview/src/components/LabelNote.vue',
oldPath: 'src/webview/src/components/LabelNote.vue',
newPath: 'src/webview/src/components/LabelNoteNew.vue',
oldPath: 'src/webview/src/components/LabelNoteOld.vue',
},
};
......
......@@ -31,7 +31,7 @@ describe('MR Review', () => {
before(async () => {
server = getServer([
createJsonEndpoint('/projects/278964/merge_requests/33824/versions', versionsResponse),
createJsonEndpoint('/projects/278964/merge_requests/33824/notes/469379582', diffNote),
createJsonEndpoint('/projects/278964/merge_requests/33824/notes/469379582', diffNote), // TODO remove
createJsonEndpoint(
'/projects/278964/merge_requests/33824/versions/127919672',
versionResponse,
......@@ -125,7 +125,7 @@ describe('MR Review', () => {
await dataProvider.getChildren(mrItemModel);
const { uri, range, comments } = thread;
assert.strictEqual(uri.path, `/${noteOnDiff.position.newPath}`);
assert.strictEqual(uri.path, `/${noteOnDiff.position.oldPath}`);
assert.strictEqual(range.start.line, noteOnDiff.position.oldLine - 1);
assert.strictEqual(comments[0].body, noteOnDiff.body);
});
......
Markdown is supported
0% .
You are about to add 0 people to the discussion. Proceed with caution.
先完成此消息的编辑!
想要评论请 注册