Merge branch '342-preliminary-refactoring' into 'main'

reafactor: prepare for creating new comments on MR diff

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

src/data_providers/items/mr_item_model.ts

@@ -74,7 +74,7 @@ export class MrItemModel extends ItemModel {
       issuable: this.mr,
     });
     const discussionsOnDiff = discussions.filter(isTextDiffDiscussion);
-    const threads = discussionsOnDiff.map(discussion => {
+    discussionsOnDiff.forEach(discussion => {
       return GitLabCommentThread.createThread({
         commentController,
         repositoryRoot: this.repository.rootFsPath,
@@ -83,6 +83,6 @@ export class MrItemModel extends ItemModel {
         gitlabService,
       });
     });
-    this.setDisposableChildren([...threads, commentController]);
+    this.setDisposableChildren([commentController]);
   }
 }

src/git/diff_line_count.ts

 import * as assert from 'assert';
+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);
@@ -37,8 +38,6 @@ type AddedLine = { type: typeof ADDED; newLine: number };
 type UnchangedLine = { type: typeof UNCHANGED; oldLine: number; newLine: number };
 type HunkLine = RemovedLine | AddedLine | UnchangedLine;
 
-type FilePath = { oldPath?: string; newPath?: string };
-
 /** Converts lines in the text hunk into data structures that represent type of the change and affected lines */
 const parseHunk = (hunk: string): HunkLine[] => {
   const [headerLine, ...remainingLines] = hunk.split('\n');
@@ -81,14 +80,8 @@ const parseHunk = (hunk: string): HunkLine[] => {
   return result.lines;
 };
 
-const getHunksForFile = (mrVersion: RestMrVersion, path: FilePath): HunkLine[][] => {
-  // VS Code Uri returns absolute path (leading slash) but GitLab uses relative paths (no leading slash)
-  const removeLeadingSlash = (filePath = ''): string => filePath.replace(/^\//, '');
-  const diff = mrVersion.diffs.find(
-    d =>
-      d.new_path === removeLeadingSlash(path.newPath) ||
-      d.old_path === removeLeadingSlash(path.oldPath),
-  );
+const getHunksForFile = (mrVersion: RestMrVersion, path: DiffFilePath): HunkLine[][] => {
+  const diff = findFileInDiffs(mrVersion.diffs, path);
   if (!diff) return [];
   return getRawHunks(diff.diff).map(parseHunk);
 };

src/git/git_extension_wrapper.test.ts

@@ -124,7 +124,7 @@ describe('GitExtensionWrapper', () => {
 
       const repository = wrapper.getRepository('/repository/root/path/');
 
-      expect(repository?.rootFsPath).toBe('/repository/root/path/');
+      expect(repository.rootFsPath).toBe('/repository/root/path/');
     });
   });
 });

src/git/git_extension_wrapper.ts

@@ -48,10 +48,12 @@ export class GitExtensionWrapper implements vscode.Disposable {
     return this.wrappedRepositories;
   }
 
-  getRepository(repositoryRoot: string): WrappedRepository | undefined {
+  getRepository(repositoryRoot: string): WrappedRepository {
     const rawRepository = this.gitApi?.getRepository(vscode.Uri.file(repositoryRoot));
-    if (!rawRepository) return undefined;
-    return this.repositories.find(r => r.hasSameRootAs(rawRepository));
+    assert(rawRepository, `Git Extension doesn't have repository with root ${repositoryRoot}`);
+    const result = this.repositories.find(r => r.hasSameRootAs(rawRepository));
+    assert(result, `GitExtensionWrapper is missing repository for ${repositoryRoot}`);
+    return result;
   }
 
   private register() {

src/gitlab/gitlab_new_service.ts

@@ -34,6 +34,7 @@ import {
   GetProjectQueryResult,
   queryGetProject,
 } from './graphql/get_project';
+import { removeLeadingSlash } from '../utils/remove_leading_slash';
 
 interface CreateNoteResult {
   createNote: {
@@ -205,8 +206,7 @@ export class GitLabNewService {
   }
 
   async getFileContent(path: string, ref: string, projectId: number): Promise<string> {
-    const pathWithoutFirstSlash = path.replace(/^\//, '');
-    const encodedPath = encodeURIComponent(pathWithoutFirstSlash);
+    const encodedPath = encodeURIComponent(removeLeadingSlash(path));
     const fileUrl = `${this.instanceUrl}/api/v4/projects/${projectId}/repository/files/${encodedPath}/raw?ref=${ref}`;
     const fileResult = await crossFetch(fileUrl, this.fetchOptions);
     if (!fileResult.ok) {

src/gitlab/graphql/shared.ts

@@ -21,6 +21,7 @@ const positionFragment = gql`
       diffRefs {
         baseSha
         headSha
+        startSha
       }
       filePath
       positionType
@@ -93,6 +94,7 @@ export interface GqlBasePosition {
   diffRefs: {
     baseSha: string;
     headSha: string;
+    startSha: string;
   };
   filePath: string;
   newPath: string;

src/gitlab_service.ts

@@ -101,7 +101,6 @@ async function fetch(
 async function fetchCurrentProject(repositoryRoot: string): Promise<GitLabProject | null> {
   try {
     const repository = gitExtensionWrapper.getRepository(repositoryRoot);
-    assert(repository, `Could not find repository in ${repositoryRoot}`);
     return (await repository.getProject()) ?? null;
   } catch (e) {
     throw new ApiError(e, 'get current project');
@@ -124,7 +123,6 @@ export async function fetchCurrentPipelineProject(
 ): Promise<GitLabProject | null> {
   try {
     const repository = gitExtensionWrapper.getRepository(repositoryRoot);
-    assert(repository, `Could not find repository in ${repositoryRoot}`);
     const { pipelineGitRemoteName } = getExtensionConfiguration();
     if (pipelineGitRemoteName) {
       const { namespace, project } = repository.getRemoteByName(pipelineGitRemoteName);
@@ -180,7 +178,7 @@ async function fetchLastPipelineForCurrentBranch(
 
   const branchName = await gitExtensionWrapper
     .getRepository(repositoryRoot)
-    ?.getTrackingBranchName();
+    .getTrackingBranchName();
   const pipelinesRootPath = `/projects/${project.restId}/pipelines`;
   const { response: pipelines } = await fetch(
     repositoryRoot,
@@ -381,7 +379,7 @@ export async function fetchOpenMergeRequestForCurrentBranch(
   const project = await fetchCurrentProject(repositoryRoot);
   const branchName = await gitExtensionWrapper
     .getRepository(repositoryRoot)
-    ?.getTrackingBranchName();
+    .getTrackingBranchName();
 
   const path = `/projects/${project?.restId}/merge_requests?state=opened&source_branch=${branchName}`;
   const { response } = await fetch(repositoryRoot, path);
@@ -441,7 +439,7 @@ export async function handlePipelineAction(action: string, repositoryRoot: strin
     if (action === 'create') {
       const branchName = await gitExtensionWrapper
         .getRepository(repositoryRoot)
-        ?.getTrackingBranchName();
+        .getTrackingBranchName();
       endpoint = `/projects/${project.restId}/pipeline?ref=${branchName}`;
     }
 

src/review/git_content_provider.ts

@@ -15,7 +15,7 @@ export class GitContentProvider implements vscode.TextDocumentContentProvider {
     const params = fromReviewUri(uri);
     if (!params.path || !params.commit) return '';
     const repository = gitExtensionWrapper.getRepository(params.repositoryRoot);
-    const result = await repository?.getFileContent(params.path, params.commit);
+    const result = await repository.getFileContent(params.path, params.commit);
     return result || provideApiContentAsFallback(uri, token);
   }
 }

src/review/gitlab_comment_thread.ts

@@ -82,7 +82,7 @@ export class GitLabCommentThread {
       return true;
     });
     if (this.vsThread.comments.length === 0) {
-      this.dispose();
+      this.vsThread.dispose();
     }
   }
 
@@ -137,10 +137,6 @@ export class GitLabCommentThread {
     }
   }
 
-  dispose(): void {
-    this.vsThread.dispose();
-  }
-
   static createThread({
     commentController,
     repositoryRoot,

src/test_utils/entities.ts

@@ -43,6 +43,7 @@ export const diffFile: RestDiffFile = {
 export const mrVersion: RestMrVersion = {
   base_commit_sha: 'aaaaaaaa',
   head_commit_sha: 'bbbbbbbb',
+  start_commit_sha: 'cccccccc',
   diffs: [diffFile],
 };
 

src/types.d.ts

@@ -25,6 +25,7 @@ interface RestIssuable {
 interface RestMrVersion {
   head_commit_sha: string;
   base_commit_sha: string;
+  start_commit_sha: string;
   diffs: RestDiffFile[];
 }
 

src/utils/find_file_in_diffs.test.ts

+import { diffFile } from '../test_utils/entities';
+import { findFileInDiffs } from './find_file_in_diffs';
+
+describe('findFileInDiffs', () => {
+  const diff: RestDiffFile = {
+    ...diffFile,
+    old_path: 'test/oldName.js',
+    new_path: 'test/newName.js',
+  };
+
+  it.each`
+    oldPath               | newPath
+    ${'/test/oldName.js'} | ${undefined}
+    ${'test/oldName.js'}  | ${undefined}
+    ${undefined}          | ${'/test/newName.js'}
+    ${undefined}          | ${'/test/newName.js'}
+  `('finds a file with oldPath: $oldPath and newPath: $newPath', ({ oldPath, newPath }) => {
+    expect(findFileInDiffs([diff], { oldPath, newPath })).toEqual(diff);
+  });
+
+  it('returns undefined when it does not find a file', () => {
+    expect(findFileInDiffs([diff], { oldPath: '/nonexistent' })).toEqual(undefined);
+  });
+});

src/utils/find_file_in_diffs.ts

+import { removeLeadingSlash } from './remove_leading_slash';
+
+export type DiffFilePath = { oldPath?: string; newPath?: string };
+
+export const findFileInDiffs = (
+  diffs: RestDiffFile[],
+  path: DiffFilePath,
+): RestDiffFile | undefined =>
+  diffs.find(
+    d =>
+      d.new_path === removeLeadingSlash(path.newPath) ||
+      d.old_path === removeLeadingSlash(path.oldPath),
+  );

src/utils/get_instance_url.ts

-import * as assert from 'assert';
 import { gitExtensionWrapper } from '../git/git_extension_wrapper';
 
 export function getInstanceUrl(repositoryRoot: string): string {
   const repository = gitExtensionWrapper.getRepository(repositoryRoot);
-  assert(repository, `${repositoryRoot} doesn't contain a git repository`);
   return repository.instanceUrl;
 }

src/utils/remove_leading_slash.ts

+/** VS Code Uri returns absolute path (leading slash) but GitLab uses relative paths (no leading slash) */
+export const removeLeadingSlash = (filePath = ''): string => filePath.replace(/^\//, '');

test/integration/fixtures/graphql/discussions.js

@@ -107,6 +107,7 @@ const noteOnDiff = {
     diffRefs: {
       baseSha: '18307069cfc96892bbe93a15249bd91babfa1064',
       headSha: 'b9c6f9ad70d55a75785fb2702ab8012a69e767d3',
+      startSha: 'b9c6f9ad70d55a75785fb2702ab8012a69e767d3',
     },
     filePath: 'src/webview/src/components/LabelNote.vue',
     positionType: 'text',