From 6ce1eebd3ad5a8b36233e027c0f1f32b05c70c32 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Tue, 19 Oct 2021 23:48:17 -0600
Subject: [PATCH 1/2] Add utility specifically for diff lines

---
 .../javascripts/diffs/utils/diff_line.js      | 10 ++++++++
 spec/frontend/diffs/utils/diff_line_spec.js   | 25 +++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 app/assets/javascripts/diffs/utils/diff_line.js
 create mode 100644 spec/frontend/diffs/utils/diff_line_spec.js

diff --git a/app/assets/javascripts/diffs/utils/diff_line.js b/app/assets/javascripts/diffs/utils/diff_line.js
new file mode 100644
index 00000000000000..b40bf1b983d168
--- /dev/null
+++ b/app/assets/javascripts/diffs/utils/diff_line.js
@@ -0,0 +1,10 @@
+export function pickDirection({ line, code } = {}) {
+  const { left, right } = line;
+  let direction = left;
+
+  if (right.line_code === code) {
+    direction = right;
+  }
+
+  return direction;
+}
diff --git a/spec/frontend/diffs/utils/diff_line_spec.js b/spec/frontend/diffs/utils/diff_line_spec.js
new file mode 100644
index 00000000000000..800872f90c2a8f
--- /dev/null
+++ b/spec/frontend/diffs/utils/diff_line_spec.js
@@ -0,0 +1,25 @@
+import { pickDirection } from '~/diffs/utils/diff_line';
+
+describe('diff_line utilities', () => {
+  describe('pickDirection', () => {
+    const left = {
+      line_code: 'left',
+    };
+    const right = {
+      line_code: 'right',
+    };
+    const line = {
+      left,
+      right,
+    };
+
+    it.each`
+      code       | pick     | pickDescription
+      ${'left'}  | ${left}  | ${'the left line'}
+      ${'right'} | ${right} | ${'the right line'}
+      ${'junk'}  | ${left}  | ${'the default: the left line'}
+    `('when provided a line and a line code `$code`, picks $pickDescription', ({ code, pick }) => {
+      expect(pickDirection({ line, code })).toBe(pick);
+    });
+  });
+});
-- 
GitLab


From 56bf3793b4de9cb2fa8c7711f1217003053d6f81 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Tue, 19 Oct 2021 23:49:10 -0600
Subject: [PATCH 2/2] Fix multiple comment forms suggesting the wrong lines

This fix basically uses a local state for "what lines is the
comment form attached to" instead of placing that info
unnecessarily in a global variable.

When the comment form is constructed, it's provided the correct
lines, and the highlighting/suggestions are created from that.

If the line range is updated manually by the user, the note form
gets the updated range and properly updates for new suggestions.

Changelog: fixed
---
 .../diffs/components/diff_comment_cell.vue    |  6 ++++++
 .../diffs/components/diff_line_note_form.vue  | 18 +++++++++++++---
 .../diffs/components/diff_view.vue            | 15 ++++++++++++-
 .../javascripts/diffs/utils/diff_line.js      |  4 ++--
 .../components/multiline_comment_form.vue     |  6 ++----
 .../notes/components/note_form.vue            |  2 +-
 .../components/diff_line_note_form_spec.js    | 13 ++++++------
 spec/frontend/diffs/utils/diff_line_spec.js   | 21 ++++++++++++-------
 .../components/multiline_comment_form_spec.js | 12 -----------
 9 files changed, 60 insertions(+), 37 deletions(-)

diff --git a/app/assets/javascripts/diffs/components/diff_comment_cell.vue b/app/assets/javascripts/diffs/components/diff_comment_cell.vue
index 4af4b46f94c7a3..a4fae652d020ee 100644
--- a/app/assets/javascripts/diffs/components/diff_comment_cell.vue
+++ b/app/assets/javascripts/diffs/components/diff_comment_cell.vue
@@ -29,6 +29,11 @@ export default {
       required: false,
       default: false,
     },
+    lineRange: {
+      type: Object,
+      required: false,
+      default: null,
+    },
     linePosition: {
       type: String,
       required: false,
@@ -59,6 +64,7 @@ export default {
         <diff-line-note-form
           :diff-file-hash="diffFileHash"
           :line="line"
+          :range="lineRange"
           :note-target-line="line"
           :help-page-path="helpPagePath"
           :line-position="linePosition"
diff --git a/app/assets/javascripts/diffs/components/diff_line_note_form.vue b/app/assets/javascripts/diffs/components/diff_line_note_form.vue
index c445989f143835..10a3e5456e0010 100644
--- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue
+++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue
@@ -32,6 +32,11 @@ export default {
       type: Object,
       required: true,
     },
+    range: {
+      type: Object,
+      required: false,
+      default: null,
+    },
     linePosition: {
       type: String,
       required: false,
@@ -49,6 +54,7 @@ export default {
   },
   data() {
     return {
+      lines: null,
       commentLineStart: {
         line_code: this.line.line_code,
         type: this.line.type,
@@ -116,10 +122,8 @@ export default {
       return commentLineOptions(lines, this.line, this.line.line_code, side);
     },
     commentLines() {
-      if (!this.selectedCommentPosition) return [];
-
       const lines = [];
-      const { start, end } = this.selectedCommentPosition;
+      const { start, end } = this.lines;
       const diffLines = this.diffFile[INLINE_DIFF_LINES_KEY];
       let isAdding = false;
 
@@ -144,6 +148,9 @@ export default {
       return lines;
     },
   },
+  created() {
+    this.lines = { ...this.range };
+  },
   mounted() {
     if (this.isLoggedIn) {
       const keys = [
@@ -189,6 +196,9 @@ export default {
         this.handleCancelCommentForm(),
       );
     },
+    updateStartLine(line) {
+      this.lines.start = line;
+    },
   },
 };
 </script>
@@ -199,7 +209,9 @@ export default {
       <multiline-comment-form
         v-model="commentLineStart"
         :line="line"
+        :line-range="lines"
         :comment-line-options="commentLineOptions"
+        @input="updateStartLine"
       />
     </div>
     <note-form
diff --git a/app/assets/javascripts/diffs/components/diff_view.vue b/app/assets/javascripts/diffs/components/diff_view.vue
index 64ded1ca8ca91b..55c796182eef89 100644
--- a/app/assets/javascripts/diffs/components/diff_view.vue
+++ b/app/assets/javascripts/diffs/components/diff_view.vue
@@ -6,6 +6,7 @@ import draftCommentsMixin from '~/diffs/mixins/draft_comments';
 import { getCommentedLines } from '~/notes/components/multiline_comment_utils';
 import { hide } from '~/tooltips';
 import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
+import { pickDirection } from '../utils/diff_line';
 import DiffCommentCell from './diff_comment_cell.vue';
 import DiffExpansionCell from './diff_expansion_cell.vue';
 import DiffRow from './diff_row.vue';
@@ -106,6 +107,16 @@ export default {
       });
       this.idState.dragStart = null;
     },
+    singleLineComment(code, line) {
+      const lineDir = pickDirection({ line, code });
+
+      this.idState.updatedLineRange = {
+        start: lineDir,
+        end: lineDir,
+      };
+
+      this.showCommentForm({ lineCode: lineDir.line_code, fileHash: this.diffFile.file_hash });
+    },
     isHighlighted(line) {
       return isHighlighted(
         this.highlightedRow,
@@ -169,7 +180,7 @@ export default {
         :index="index"
         :is-highlighted="isHighlighted(line)"
         :file-line-coverage="fileLineCoverage"
-        @showCommentForm="(lineCode) => showCommentForm({ lineCode, fileHash: diffFile.file_hash })"
+        @showCommentForm="(code) => singleLineComment(code, line)"
         @setHighlightedRow="setHighlightedRow"
         @toggleLineDiscussions="
           ({ lineCode, expanded }) =>
@@ -193,6 +204,7 @@ export default {
           <diff-comment-cell
             v-if="line.left && (line.left.renderDiscussion || line.left.hasCommentForm)"
             :line="line.left"
+            :line-range="idState.updatedLineRange"
             :diff-file-hash="diffFile.file_hash"
             :help-page-path="helpPagePath"
             line-position="left"
@@ -206,6 +218,7 @@ export default {
           <diff-comment-cell
             v-if="line.right && (line.right.renderDiscussion || line.right.hasCommentForm)"
             :line="line.right"
+            :line-range="idState.updatedLineRange"
             :diff-file-hash="diffFile.file_hash"
             :line-index="index"
             :help-page-path="helpPagePath"
diff --git a/app/assets/javascripts/diffs/utils/diff_line.js b/app/assets/javascripts/diffs/utils/diff_line.js
index b40bf1b983d168..a248cc6318bd23 100644
--- a/app/assets/javascripts/diffs/utils/diff_line.js
+++ b/app/assets/javascripts/diffs/utils/diff_line.js
@@ -1,8 +1,8 @@
 export function pickDirection({ line, code } = {}) {
   const { left, right } = line;
-  let direction = left;
+  let direction = left || right;
 
-  if (right.line_code === code) {
+  if (right?.line_code === code) {
     direction = right;
   }
 
diff --git a/app/assets/javascripts/notes/components/multiline_comment_form.vue b/app/assets/javascripts/notes/components/multiline_comment_form.vue
index 6ad565567be5bb..1633b79c3be463 100644
--- a/app/assets/javascripts/notes/components/multiline_comment_form.vue
+++ b/app/assets/javascripts/notes/components/multiline_comment_form.vue
@@ -1,6 +1,6 @@
 <script>
 import { GlFormSelect, GlSprintf } from '@gitlab/ui';
-import { mapActions, mapState } from 'vuex';
+import { mapActions } from 'vuex';
 import { getSymbol, getLineClasses } from './multiline_comment_utils';
 
 export default {
@@ -27,13 +27,12 @@ export default {
     };
   },
   computed: {
-    ...mapState({ selectedCommentPosition: ({ notes }) => notes.selectedCommentPosition }),
     lineNumber() {
       return this.commentLineOptions[this.commentLineOptions.length - 1].text;
     },
   },
   created() {
-    const line = this.selectedCommentPosition?.start || this.lineRange?.start || this.line;
+    const line = this.lineRange?.start || this.line;
 
     this.commentLineStart = {
       line_code: line.line_code,
@@ -42,7 +41,6 @@ export default {
       new_line: line.new_line,
     };
 
-    if (this.selectedCommentPosition) return;
     this.highlightSelection();
   },
   destroyed() {
diff --git a/app/assets/javascripts/notes/components/note_form.vue b/app/assets/javascripts/notes/components/note_form.vue
index b05643e5e13c33..5129fd1317df0e 100644
--- a/app/assets/javascripts/notes/components/note_form.vue
+++ b/app/assets/javascripts/notes/components/note_form.vue
@@ -334,13 +334,13 @@ export default {
           :markdown-docs-path="markdownDocsPath"
           :quick-actions-docs-path="quickActionsDocsPath"
           :line="line"
+          :lines="lines"
           :note="discussionNote"
           :can-suggest="canSuggest"
           :add-spacing-classes="false"
           :help-page-path="helpPagePath"
           :show-suggest-popover="showSuggestPopover"
           :textarea-value="updatedNoteBody"
-          :lines="lines"
           @handleSuggestDismissed="() => $emit('handleSuggestDismissed')"
         >
           <template #textarea>
diff --git a/spec/frontend/diffs/components/diff_line_note_form_spec.js b/spec/frontend/diffs/components/diff_line_note_form_spec.js
index a192f7e2e9a3c6..5132cfd76227c6 100644
--- a/spec/frontend/diffs/components/diff_line_note_form_spec.js
+++ b/spec/frontend/diffs/components/diff_line_note_form_spec.js
@@ -26,8 +26,9 @@ describe('DiffLineNoteForm', () => {
       propsData: {
         diffFileHash: diffFile.file_hash,
         diffLines,
-        line: diffLines[0],
-        noteTargetLine: diffLines[0],
+        line: diffLines[1],
+        range: { start: diffLines[0], end: diffLines[1] },
+        noteTargetLine: diffLines[1],
       },
     });
   };
@@ -67,7 +68,7 @@ describe('DiffLineNoteForm', () => {
         expect(window.confirm).not.toHaveBeenCalled();
         wrapper.vm.$nextTick(() => {
           expect(wrapper.vm.cancelCommentForm).toHaveBeenCalledWith({
-            lineCode: diffLines[0].line_code,
+            lineCode: diffLines[1].line_code,
             fileHash: wrapper.vm.diffFileHash,
           });
 
@@ -88,13 +89,13 @@ describe('DiffLineNoteForm', () => {
           start: {
             line_code: wrapper.vm.commentLineStart.line_code,
             type: wrapper.vm.commentLineStart.type,
-            new_line: 1,
+            new_line: 2,
             old_line: null,
           },
           end: {
             line_code: wrapper.vm.line.line_code,
             type: wrapper.vm.line.type,
-            new_line: 1,
+            new_line: 2,
             old_line: null,
           },
         };
@@ -120,7 +121,7 @@ describe('DiffLineNoteForm', () => {
 
   describe('mounted', () => {
     it('should init autosave', () => {
-      const key = 'autosave/Note/Issue/98//DiffNote//1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1';
+      const key = 'autosave/Note/Issue/98//DiffNote//1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2';
       wrapper = createComponent();
 
       expect(wrapper.vm.autosave).toBeDefined();
diff --git a/spec/frontend/diffs/utils/diff_line_spec.js b/spec/frontend/diffs/utils/diff_line_spec.js
index 800872f90c2a8f..adcb4a4433c02d 100644
--- a/spec/frontend/diffs/utils/diff_line_spec.js
+++ b/spec/frontend/diffs/utils/diff_line_spec.js
@@ -8,18 +8,23 @@ describe('diff_line utilities', () => {
     const right = {
       line_code: 'right',
     };
-    const line = {
+    const defaultLine = {
       left,
       right,
     };
 
     it.each`
-      code       | pick     | pickDescription
-      ${'left'}  | ${left}  | ${'the left line'}
-      ${'right'} | ${right} | ${'the right line'}
-      ${'junk'}  | ${left}  | ${'the default: the left line'}
-    `('when provided a line and a line code `$code`, picks $pickDescription', ({ code, pick }) => {
-      expect(pickDirection({ line, code })).toBe(pick);
-    });
+      code       | pick     | line           | pickDescription
+      ${'left'}  | ${left}  | ${defaultLine} | ${'the left line'}
+      ${'right'} | ${right} | ${defaultLine} | ${'the right line'}
+      ${'junk'}  | ${left}  | ${defaultLine} | ${'the default: the left line'}
+      ${'junk'}  | ${right} | ${{ right }}   | ${"the right line if there's no left line to default to"}
+      ${'right'} | ${left}  | ${{ left }}    | ${"the left line when there isn't a right line to match"}
+    `(
+      'when provided a line and a line code `$code`, picks $pickDescription',
+      ({ code, line, pick }) => {
+        expect(pickDirection({ line, code })).toBe(pick);
+      },
+    );
   });
 });
diff --git a/spec/frontend/notes/components/multiline_comment_form_spec.js b/spec/frontend/notes/components/multiline_comment_form_spec.js
index b6d603c6358ec4..b027a261c151c3 100644
--- a/spec/frontend/notes/components/multiline_comment_form_spec.js
+++ b/spec/frontend/notes/components/multiline_comment_form_spec.js
@@ -50,18 +50,6 @@ describe('MultilineCommentForm', () => {
       expect(wrapper.vm.commentLineStart).toEqual(lineRange.start);
       expect(setSelectedCommentPosition).toHaveBeenCalled();
     });
-
-    it('sets commentLineStart to selectedCommentPosition', () => {
-      const notes = {
-        selectedCommentPosition: {
-          start: { ...testLine },
-        },
-      };
-      const wrapper = createWrapper({}, { notes });
-
-      expect(wrapper.vm.commentLineStart).toEqual(wrapper.vm.selectedCommentPosition.start);
-      expect(setSelectedCommentPosition).not.toHaveBeenCalled();
-    });
   });
 
   describe('destroyed', () => {
-- 
GitLab