refactor(mr review): local comment edit can't cause data loss

If the comment changed on the server, but the extension still
shows the old version, we'll prevent the extension from overriding
the server's latest version. We'll warn the user that the
comment has changed since they last saw it.

src/data_providers/items/mr_item_model.ts

@@ -71,7 +71,7 @@ export class MrItemModel extends ItemModel {
       return GitLabCommentThread.createThread({
         commentController,
         workspaceFolder: this.workspace.uri,
-        gitlabProjectId: this.mr.project_id,
+        mr: this.mr,
         discussion,
         gitlabService,
       });

src/gitlab/gitlab_new_service.ts

@@ -141,6 +141,10 @@ interface GetDiscussionsOptions {
   endCursor?: string;
 }
 
+interface RestNote {
+  body: string;
+}
+
 function isLabelEvent(note: Note): note is RestLabelEvent {
   return (note as RestLabelEvent).label !== undefined;
 }
@@ -559,10 +563,42 @@ export class GitLabNewService {
     }
   }
 
-  async updateNoteBody(noteId: string, body: string): Promise<void> {
+  /**
+   * This method is used only as a replacement of optimistic locking when updating a note.
+   * We request the latest note to validate that it hasn't changed since we last saw it.
+   */
+  private async getMrNote(mr: RestIssuable, noteId: number): Promise<RestNote> {
+    const noteUrl = `${this.instanceUrl}/api/v4/projects/${mr.project_id}/merge_requests/${mr.iid}/notes/${noteId}`;
+    const result = await crossFetch(noteUrl, this.fetchOptions);
+    if (!result.ok) {
+      throw new FetchError(`Fetching the latest note from ${noteUrl} failed`, result);
+    }
+    return result.json();
+  }
+
+  async updateNoteBody(
+    noteGqlId: string,
+    body: string,
+    originalBody: string,
+    mr: RestIssuable,
+  ): Promise<void> {
+    const latestNote = await this.getMrNote(mr, getRestIdFromGraphQLId(noteGqlId));
+    // This check is the best workaround we can do in the lack of optimistic locking
+    // Issue to make this check in the GitLab instance: https://gitlab.com/gitlab-org/gitlab/-/issues/323808
+    if (latestNote.body !== originalBody) {
+      throw new UserFriendlyError(
+        `This comment changed after you last viewed it, and can't be edited.
+        Your new comment is NOT lost. To retrieve it, edit the comment again and copy your comment text,
+        then update the original comment by opening the sidebar and running the
+        "GitLab: Refresh sidebar" command.`,
+        new Error(
+          `You last saw:\n"${originalBody}"\nbut the latest version is:\n"${latestNote.body}"`,
+        ),
+      );
+    }
     try {
       await this.client.request<void>(updateNoteBodyMutation, {
-        noteId,
+        noteId: noteGqlId,
         body,
       });
     } catch (e) {

src/review/gitlab_comment_thread.test.ts

@@ -10,6 +10,7 @@ import {
   GqlTextDiffDiscussion,
   GqlTextDiffNote,
 } from '../gitlab/gitlab_new_service';
+import { mr } from '../test_utils/entities';
 
 describe('GitLabCommentThread', () => {
   let gitlabCommentThread: GitLabCommentThread;
@@ -66,7 +67,7 @@ describe('GitLabCommentThread', () => {
     gitlabCommentThread = GitLabCommentThread.createThread({
       commentController: fakeCommentController,
       workspaceFolder: '/workspaceFolder',
-      gitlabProjectId: 12345,
+      mr,
       discussion,
       gitlabService,
     });

src/review/gitlab_comment_thread.ts

@@ -33,7 +33,7 @@ const uriFromPosition = (
 interface CreateThreadOptions {
   commentController: vscode.CommentController;
   workspaceFolder: string;
-  gitlabProjectId: number;
+  mr: RestIssuable;
   discussion: GqlTextDiffDiscussion;
   gitlabService: GitLabNewService;
 }
@@ -45,6 +45,7 @@ export class GitLabCommentThread {
     private vsThread: vscode.CommentThread,
     private gqlDiscussion: GqlTextDiffDiscussion,
     private gitlabService: GitLabNewService,
+    private mr: RestIssuable,
   ) {
     this.vsThread.collapsibleState = vscode.CommentThreadCollapsibleState.Expanded;
     this.vsThread.canReply = false;
@@ -84,7 +85,12 @@ export class GitLabCommentThread {
   }
 
   async submitEdit(comment: GitLabComment): Promise<void> {
-    await this.gitlabService.updateNoteBody(comment.id, comment.body);
+    await this.gitlabService.updateNoteBody(
+      comment.id,
+      comment.body,
+      comment.gqlNote.body, // this is what we think is the latest version stored in API
+      this.mr,
+    );
     this.changeOneComment(comment.id, c =>
       c.markBodyAsSubmitted().withMode(vscode.CommentMode.Preview),
     );
@@ -115,19 +121,19 @@ export class GitLabCommentThread {
   static createThread({
     commentController,
     workspaceFolder,
-    gitlabProjectId,
+    mr,
     discussion,
     gitlabService,
   }: CreateThreadOptions): GitLabCommentThread {
     const { position } = discussion.notes.nodes[0];
     const vsThread = commentController.createCommentThread(
-      uriFromPosition(position, workspaceFolder, gitlabProjectId),
+      uriFromPosition(position, workspaceFolder, mr.project_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);
+    const glThread = new GitLabCommentThread(vsThread, discussion, gitlabService, mr);
     vsThread.comments = discussion.notes.nodes.map(note =>
       GitLabComment.fromGqlNote(note, glThread),
     );

test/integration/fixtures/rest/diff_note.json

+{
+  "id": 469379582,
+  "type": "DiffNote",
+  "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.",
+  "attachment": null,
+  "author": {
+    "id": 3457201,
+    "name": "Tomas Vik",
+    "username": "viktomas",
+    "state": "active",
+    "avatar_url": "https://secure.gravatar.com/avatar/6042a9152ada74d9fb6a0cdce895337e?s=80&d=identicon",
+    "web_url": "https://gitlab.com/viktomas"
+  },
+  "created_at": "2021-03-08T14:19:18.296Z",
+  "updated_at": "2021-03-08T14:57:27.990Z",
+  "system": false,
+  "noteable_id": 87196674,
+  "noteable_type": "MergeRequest",
+  "commit_id": null,
+  "position": {
+    "base_sha": "5e6dffa282c5129aa67cd227a0429be21bfdaf80",
+    "start_sha": "5e6dffa282c5129aa67cd227a0429be21bfdaf80",
+    "head_sha": "a2616e0fea06c8b5188e4064a76d623afbf97b0c",
+    "old_path": "test.js",
+    "new_path": "test.ts",
+    "position_type": "text",
+    "old_line": null,
+    "new_line": 19,
+    "line_range": {
+      "start": {
+        "line_code": "5f7b6aabace88f74752a2b616628e855914e4bc7_19_19",
+        "type": "new",
+        "old_line": null,
+        "new_line": 19
+      },
+      "end": {
+        "line_code": "5f7b6aabace88f74752a2b616628e855914e4bc7_19_19",
+        "type": "new",
+        "old_line": null,
+        "new_line": 19
+      }
+    }
+  },
+  "resolvable": true,
+  "resolved": false,
+  "resolved_by": null,
+  "resolved_at": null,
+  "confidential": false,
+  "noteable_iid": 5,
+  "commands_changes": {}
+}

test/integration/mr_review.test.js

@@ -6,9 +6,11 @@ const { graphql } = require('msw');
 const IssuableDataProvider = require('../../src/data_providers/issuable').DataProvider;
 const { MrItemModel } = require('../../src/data_providers/items/mr_item_model');
 const { tokenService } = require('../../src/services/token_service');
+const { submitEdit } = require('../../src/commands/mr_discussion_commands');
 const openMergeRequestResponse = require('./fixtures/rest/open_mr.json');
 const versionsResponse = require('./fixtures/rest/versions.json');
 const versionResponse = require('./fixtures/rest/mr_version.json');
+const diffNote = require('./fixtures/rest/diff_note.json');
 const { projectWithMrDiscussions, noteOnDiff } = require('./fixtures/graphql/discussions');
 const {
   getServer,
@@ -28,6 +30,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/versions/127919672',
         versionResponse,
@@ -116,6 +119,19 @@ describe('MR Review', () => {
       assert.strictEqual(range.start.line, noteOnDiff.position.oldLine - 1);
       assert.strictEqual(comments[0].body, noteOnDiff.body);
     });
+
+    it('editing comment fails if the comment body has changed on the GitLab instance', async () => {
+      await dataProvider.getChildren(mrItemModel);
+      const [firstComment] = thread.comments;
+      firstComment.gqlNote.body =
+        'this body simulates that our version of the note body is out of sync with the GitLab instance';
+      firstComment.body = 'user wants to change the body to this';
+
+      await assert.rejects(
+        submitEdit(firstComment),
+        /This comment changed after you last viewed it, and can't be edited/,
+      );
+    });
   });
 
   describe('clicking on a changed file', () => {