Merge branch '266-show-comments-on-mr-diff' into 'main'

feat(mr review): show comments on changed file diff

See merge request gitlab-org/gitlab-vscode-extension!158

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": {

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",

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 };
   },
 };

src/data_providers/items/changed_file_item.test.ts

 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);
       },
     );

src/data_providers/items/mr_item_model.test.ts

+/* 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);
+  });
+});

src/data_providers/items/mr_item_model.ts

 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<void> {
+    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<vscode.TreeItem[]> {
     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`);
-  }
 }

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<GqlNote>;
@@ -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<GqlDiscussion[]> {
+  async getDiscussions(issuable: RestIssuable, endCursor?: string): Promise<GqlDiscussion[]> {
     const projectPath = getProjectPath(issuable);
     const query = constructGetDiscussionsQuery(isMr(issuable));
     const result = await this.client.request<GqlProjectResult<GqlDiscussionsProject>>(query, {

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;

src/test_utils/entities.ts

 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',

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:
+    '<p data-sourcepos="1:1-1:210" dir="auto">This is the core improvement. <code>NoteBody</code> sends the <code>note.body</code> to our <code>/api/v4/markdown</code> endpoint to render HTML. For labels, we can easily render the HTML ourselves, saving all the API requests and complexity.</p>',
+  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,
 };