diff --git a/package-lock.json b/package-lock.json index c9bcacbce59196d121067eca9993f42f34192725..35c7e69608c679b5e0c75a204c7656bcfc04b82f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1559,9 +1559,9 @@ "dev": true }, "@types/vscode": { - "version": "1.46.0", - "resolved": "https://registry.npmjs.org/@types/vscode/-/vscode-1.46.0.tgz", - "integrity": "sha512-8m9wPEB2mcRqTWNKs9A9Eqs8DrQZt0qNFO8GkxBOnyW6xR//3s77SoMgb/nY1ctzACsZXwZj3YRTDsn4bAoaUw==", + "version": "1.51.0", + "resolved": "https://registry.npmjs.org/@types/vscode/-/vscode-1.51.0.tgz", + "integrity": "sha512-C/jZ35OT5k/rsJyAK8mS1kM++vMcm89oSWegkzxRCvHllIq0cToZAkIDs6eCY4SKrvik3nrhELizyLcM0onbQA==", "dev": true }, "@types/yargs": { diff --git a/package.json b/package.json index 3620ffc725b3d1df2aad9cb4aa2077c2f4d46849..9d91602042cdb409abccc62d138d8d51dc5acbfb 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,7 @@ "url": "https://gitlab.com/gitlab-org/gitlab-vscode-extension" }, "engines": { - "vscode": "^1.41.0" + "vscode": "^1.51.0" }, "categories": [ "Other" @@ -521,7 +521,7 @@ "@types/request-promise": "^4.1.46", "@types/sinon": "^9.0.4", "@types/temp": "^0.8.34", - "@types/vscode": "^1.41.0", + "@types/vscode": "^1.51.0", "@typescript-eslint/eslint-plugin": "^3.7.0", "@typescript-eslint/parser": "^3.7.0", "conventional-changelog-cli": "^2.0.34", diff --git a/src/__mocks__/vscode.js b/src/__mocks__/vscode.js index bec96194228560bb128c290f0254e1245be6e520..2530585001e1a1803c2ff9ec5eaa3b0c0064074f 100644 --- a/src/__mocks__/vscode.js +++ b/src/__mocks__/vscode.js @@ -12,7 +12,22 @@ module.exports = { Uri: { file: path => ({ path, - with: jest.fn(), + with: (...args) => ({ + path, + args, + }), }), + parse: str => str, + }, + comments: { + createCommentController: jest.fn(), + }, + CommentMode: { Preview: 1 }, + CommentThreadCollapsibleState: { Expanded: 1 }, + Position: function Position(x, y) { + return { x, y }; + }, + Range: function Range(start, end) { + return { start, end }; }, }; diff --git a/src/data_providers/items/changed_file_item.test.ts b/src/data_providers/items/changed_file_item.test.ts index 8896ff9561f5e14a2e80524dfb608505df8051af..7af4516c9f4b4544bc0f2f182759779afd9086c9 100644 --- a/src/data_providers/items/changed_file_item.test.ts +++ b/src/data_providers/items/changed_file_item.test.ts @@ -1,5 +1,5 @@ import { PROGRAMMATIC_COMMANDS } from '../../command_names'; -import { diffFile, issuable, mrVersion, project } from '../../test_utils/entities'; +import { diffFile, issue, mrVersion, project } from '../../test_utils/entities'; import { ChangedFileItem } from './changed_file_item'; describe('ChangedFileItem', () => { @@ -8,7 +8,7 @@ describe('ChangedFileItem', () => { 'should not show diff for %s', extension => { const changedImageFile = { ...diffFile, new_path: `file${extension}` }; - const item = new ChangedFileItem(issuable, mrVersion, changedImageFile, project); + const item = new ChangedFileItem(issue, mrVersion, changedImageFile, project); expect(item.command?.command).toBe(PROGRAMMATIC_COMMANDS.NO_IMAGE_REVIEW); }, ); diff --git a/src/data_providers/items/mr_item_model.test.ts b/src/data_providers/items/mr_item_model.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..1cffcbd539b78724e06f6be2f60c1f70f8f2f9d4 --- /dev/null +++ b/src/data_providers/items/mr_item_model.test.ts @@ -0,0 +1,51 @@ +/* eslint-disable import/first */ +jest.mock('../../service_factory'); + +import * as vscode from 'vscode'; +import { MrItemModel } from './mr_item_model'; +import { mr, project } from '../../test_utils/entities'; +import { + discussionOnDiff, + noteOnDiffTextSnippet, + multipleNotes, +} from '../../../test/integration/fixtures/graphql/discussions.js'; +import { createGitLabNewService } from '../../service_factory'; + +const createCommentControllerMock = vscode.comments.createCommentController as jest.Mock; +const createGitLabNewServiceMock = createGitLabNewService as jest.Mock; + +describe('MrItemModel', () => { + let item: MrItemModel; + + const createCommentThreadMock = jest.fn(); + + beforeEach(() => { + item = new MrItemModel(mr, project); + + createCommentControllerMock.mockReturnValue({ + createCommentThread: createCommentThreadMock.mockReturnValue({}), + }); + createGitLabNewServiceMock.mockReturnValue({ + getDiscussions: jest.fn().mockResolvedValue([discussionOnDiff, multipleNotes]), + getMrDiff: jest.fn().mockResolvedValue({ diffs: [] }), + }); + }); + + afterEach(() => { + createCommentControllerMock.mockReset(); + createCommentThreadMock.mockReset(); + }); + + it('should add comment thread to VS Code', async () => { + await item.getChildren(); + expect(createCommentControllerMock).toBeCalledWith('gitlab-org/gitlab!2000', 'Issuable Title'); + const [uri, range, comments] = createCommentThreadMock.mock.calls[0]; + expect(uri.path).toBe('src/webview/src/components/LabelNote.vue'); + expect(range.start.x).toBe(47); + expect(comments.length).toBe(2); + const firstComment = comments[0]; + expect(firstComment.author.name).toBe('Tomas Vik'); + expect(firstComment.mode).toBe(vscode.CommentMode.Preview); + expect(firstComment.body).toMatch(noteOnDiffTextSnippet); + }); +}); diff --git a/src/data_providers/items/mr_item_model.ts b/src/data_providers/items/mr_item_model.ts index c1c2f9fd168b833ab972ff9fcd9b45c991567e51..362185626283019583015434bea0008601cfc72b 100644 --- a/src/data_providers/items/mr_item_model.ts +++ b/src/data_providers/items/mr_item_model.ts @@ -1,9 +1,24 @@ import * as vscode from 'vscode'; +import * as assert from 'assert'; import { PROGRAMMATIC_COMMANDS } from '../../command_names'; -import { log } from '../../log'; +import { toReviewUri } from '../../review/review_uri'; import { createGitLabNewService } from '../../service_factory'; import { ChangedFileItem } from './changed_file_item'; import { ItemModel } from './item_model'; +import { GqlDiscussion, GqlPosition } from '../../gitlab/gitlab_new_service'; +import { handleError } from '../../log'; + +const containsTextPosition = (discussion: GqlDiscussion): boolean => { + const firstNote = discussion.notes.nodes[0]; + return firstNote?.position?.positionType === 'text'; +}; + +const commentRangeFromPosition = (position: GqlPosition): vscode.Range => { + const glLine = position.oldLine || position.newLine; + assert(glLine, 'there is always eitehr new or old line'); + const vsPosition = new vscode.Position(glLine - 1, 0); + return new vscode.Range(vsPosition, vsPosition); +}; export class MrItemModel extends ItemModel { constructor(readonly mr: RestIssuable, readonly project: VsProject) { @@ -28,17 +43,62 @@ export class MrItemModel extends ItemModel { arguments: [this.mr, this.project.uri], title: 'Show MR', }; + try { + await this.getMrDiscussions(); + } catch (e) { + handleError(e); + } const changedFiles = await this.getChangedFiles(); return [description, ...changedFiles]; } + private uriFromPosition(position: GqlPosition): vscode.Uri { + const onOldVersion = Boolean(position.oldLine); + const path = onOldVersion ? position.oldPath : position.newPath; + const commit = onOldVersion ? position.diffRefs.baseSha : position.diffRefs.headSha; + return toReviewUri({ + path, + commit, + workspacePath: this.project.uri, + projectId: this.mr.project_id, + }); + } + + private async getMrDiscussions(): Promise { + const commentController = vscode.comments.createCommentController( + this.mr.references.full, + this.mr.title, + ); + + const gitlabService = await createGitLabNewService(this.project.uri); + + const discussions = await gitlabService.getDiscussions(this.mr); + const discussionsOnDiff = discussions.filter(containsTextPosition); + const threads = discussionsOnDiff.map(({ notes }) => { + const comments = notes.nodes.map(({ body, author }) => ({ + body, + mode: vscode.CommentMode.Preview, + author: { + name: author.name, + iconPath: vscode.Uri.parse(author.avatarUrl), + }, + })); + const position = notes.nodes[0]?.position as GqlPosition; // we filtered out all discussions without position + const thread = commentController.createCommentThread( + this.uriFromPosition(position), + commentRangeFromPosition(position), + comments, + ); + thread.collapsibleState = vscode.CommentThreadCollapsibleState.Expanded; + thread.canReply = false; + return thread; + }); + this.setDisposableChildren([...threads, commentController]); + } + private async getChangedFiles(): Promise { const gitlabService = await createGitLabNewService(this.project.uri); const mrVersion = await gitlabService.getMrDiff(this.mr); return mrVersion.diffs.map(d => new ChangedFileItem(this.mr, mrVersion, d, this.project)); } - - dispose(): void { - log(`MR ${this.mr.title} item model got disposed`); - } } diff --git a/src/gitlab/gitlab_new_service.ts b/src/gitlab/gitlab_new_service.ts index 4878d6f0f216d3db68d03187ff3f436b9d15430d..019c13c287025a596688184dadd6d837d5c32960 100644 --- a/src/gitlab/gitlab_new_service.ts +++ b/src/gitlab/gitlab_new_service.ts @@ -51,8 +51,22 @@ interface GqlNote { system: boolean; body: string; // TODO: remove this once the SystemNote.vue doesn't require plain text body bodyHtml: string; + position?: GqlPosition; } -interface GqlDiscussion { + +export interface GqlPosition { + diffRefs: { + baseSha: string; + headSha: string; + }; + filePath: string; + positionType: string; + newLine: number | null; + oldLine: number | null; + newPath: string; + oldPath: string; +} +export interface GqlDiscussion { replyId: string; createdAt: string; notes: Node; @@ -127,6 +141,18 @@ const discussionsFragment = gql` } body bodyHtml + position { + diffRefs { + baseSha + headSha + } + filePath + positionType + newLine + oldLine + newPath + oldPath + } } } } @@ -268,10 +294,7 @@ export class GitLabNewService { }; } - private async getDiscussions( - issuable: RestIssuable, - endCursor?: string, - ): Promise { + async getDiscussions(issuable: RestIssuable, endCursor?: string): Promise { const projectPath = getProjectPath(issuable); const query = constructGetDiscussionsQuery(isMr(issuable)); const result = await this.client.request>(query, { diff --git a/src/log.ts b/src/log.ts index fe0f446577c51dc83adcc801ad3555510ffd37e8..1c0bf3e1d771387f33dd4d6b17825a5dca903af8 100644 --- a/src/log.ts +++ b/src/log.ts @@ -8,7 +8,7 @@ function isDetailedError(object: any): object is IDetailedError { type logFunction = (line: string) => void; -let globalLog: logFunction; +let globalLog: logFunction = console.error; export const initializeLogging = (logLine: logFunction): void => { globalLog = logLine; diff --git a/src/test_utils/entities.ts b/src/test_utils/entities.ts index d1476aaf93d7908c2750ad4c042148efdeb4be03..e0177932170cb2d1c15d4fc9f21dd22129f9af7d 100644 --- a/src/test_utils/entities.ts +++ b/src/test_utils/entities.ts @@ -1,6 +1,6 @@ import { CustomQueryType } from '../gitlab/custom_query_type'; -export const issuable: RestIssuable = { +export const issue: RestIssuable = { id: 1, iid: 1000, title: 'Issuable Title', @@ -16,6 +16,16 @@ export const issuable: RestIssuable = { }, }; +export const mr: RestIssuable = { + ...issue, + id: 2, + iid: 2000, + web_url: 'https://gitlab.example.com/group/project/merge_requests//1', + references: { + full: 'gitlab-org/gitlab!2000', + }, +}; + export const diffFile: RestDiffFile = { old_path: 'old_file.js', new_path: 'new_file.js', diff --git a/test/integration/fixtures/graphql/discussions.js b/test/integration/fixtures/graphql/discussions.js index cbc597b348d8f7711e7aaccbe2770060cecadd19..89b0f78ab961fb63d8078aacac687980658b5129 100644 --- a/test/integration/fixtures/graphql/discussions.js +++ b/test/integration/fixtures/graphql/discussions.js @@ -68,6 +68,38 @@ const note2 = { const note2TextSnippet = 'I know that the dependencies are managed separately, but it would'; +const noteOnDiff = { + id: 'gid://gitlab/DiffNote/469379582', + createdAt: '2020-12-17T17:20:14Z', + system: false, + author: { + avatarUrl: + 'https://secure.gravatar.com/avatar/6042a9152ada74d9fb6a0cdce895337e?s=80&d=identicon', + name: 'Tomas Vik', + username: 'viktomas', + webUrl: 'https://gitlab.com/viktomas', + }, + body: + 'This is the core improvement. `NoteBody` sends the `note.body` to our `/api/v4/markdown` endpoint to render HTML. For labels, we can easily render the HTML ourselves, saving all the API requests and complexity.', + bodyHtml: + '

This is the core improvement. NoteBody sends the note.body to our /api/v4/markdown endpoint to render HTML. For labels, we can easily render the HTML ourselves, saving all the API requests and complexity.

', + position: { + diffRefs: { + baseSha: '18307069cfc96892bbe93a15249bd91babfa1064', + headSha: 'b9c6f9ad70d55a75785fb2702ab8012a69e767d3', + }, + filePath: 'src/webview/src/components/LabelNote.vue', + positionType: 'text', + newLine: null, + oldLine: 48, + newPath: 'src/webview/src/components/LabelNote.vue', + oldPath: 'src/webview/src/components/LabelNote.vue', + }, +}; + +const noteOnDiffTextSnippet = + 'For labels, we can easily render the HTML ourselves, saving all the API requests'; + const singleNote = { replyId: 'gid://gitlab/IndividualNoteDiscussion/afbf8f461a773fc130aa8091c6636f22efb5f4c5', createdAt: '2020-12-02T17:00:04Z', @@ -92,6 +124,18 @@ const multipleNotes = { }, }; +const discussionOnDiff = { + replyId: 'gid://gitlab/DiffDiscussion/9a702bfa62ab0a6e7c1bee74444086567e5e99e6', + createdAt: '2020-12-17T17:20:14Z', + notes: { + pageInfo: { + hasNextPage: false, + endCursor: 'MQ', + }, + nodes: [noteOnDiff, note2], + }, +}; + const projectWithDiscussions = { project: { id: 'gid://gitlab/Project/278964', @@ -113,8 +157,11 @@ module.exports = { note1TextSnippet, note2, note2TextSnippet, + noteOnDiff, + noteOnDiffTextSnippet, singleNote, multipleNotes, + discussionOnDiff, systemNote, systemNoteTextSnippet, };