Merge branch 'jdb/mutliline-comment-fe-parallel' into 'master'

Fix issues with MLC in parallel mode

See merge request gitlab-org/gitlab!33655

app/assets/javascripts/batch_comments/components/draft_note.vue

@@ -71,8 +71,8 @@ export default {
     <ul class="notes draft-notes">
       <noteable-note
         :note="draft"
-        :diff-lines="diffFile.highlighted_diff_lines"
         :line="line"
+        :discussion-root="true"
         class="draft-note"
         @handleEdit="handleEditing"
         @cancelForm="handleNotEditing"

app/assets/javascripts/batch_comments/components/parallel_draft_comment_row.vue

@@ -35,11 +35,15 @@ export default {
   <tr :class="className" class="notes_holder">
     <td class="notes_line old"></td>
     <td class="notes-content parallel old" colspan="2">
-      <div v-if="leftDraft.isDraft" class="content"><draft-note :draft="leftDraft" /></div>
+      <div v-if="leftDraft.isDraft" class="content">
+        <draft-note :draft="leftDraft" :line="line.left" />
+      </div>
     </td>
     <td class="notes_line new"></td>
     <td class="notes-content parallel new" colspan="2">
-      <div v-if="rightDraft.isDraft" class="content"><draft-note :draft="rightDraft" /></div>
+      <div v-if="rightDraft.isDraft" class="content">
+        <draft-note :draft="rightDraft" :line="line.right" />
+      </div>
     </td>
   </tr>
 </template>

app/assets/javascripts/batch_comments/components/preview_item.vue

@@ -52,14 +52,12 @@ export default {
       });
     },
     linePosition() {
-      if (this.draft.position && this.draft.position.position_type === IMAGE_DIFF_POSITION_TYPE) {
+      if (this.position?.position_type === IMAGE_DIFF_POSITION_TYPE) {
         // eslint-disable-next-line @gitlab/require-i18n-strings
-        return `${this.draft.position.x}x ${this.draft.position.y}y`;
+        return `${this.position.x}x ${this.position.y}y`;
       }
 
-      const position = this.discussion ? this.discussion.position : this.draft.position;
-
-      return position?.new_line || position?.old_line;
+      return this.position?.new_line || this.position?.old_line;
     },
     content() {
       const el = document.createElement('div');
@@ -70,11 +68,14 @@ export default {
     showLinePosition() {
       return this.draft.file_hash || this.isDiffDiscussion;
     },
+    position() {
+      return this.draft.position || this.discussion.position;
+    },
     startLineNumber() {
-      return getStartLineNumber(this.draft.position?.line_range);
+      return getStartLineNumber(this.position?.line_range);
     },
     endLineNumber() {
-      return getEndLineNumber(this.draft.position?.line_range);
+      return getEndLineNumber(this.position?.line_range);
     },
   },
   methods: {

app/assets/javascripts/diffs/components/diff_line_note_form.vue

@@ -8,7 +8,10 @@ import MultilineCommentForm from '../../notes/components/multiline_comment_form.
 import autosave from '../../notes/mixins/autosave';
 import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue';
 import { DIFF_NOTE_TYPE } from '../constants';
-import { commentLineOptions } from '../../notes/components/multiline_comment_utils';
+import {
+  commentLineOptions,
+  formatLineRange,
+} from '../../notes/components/multiline_comment_utils';
 
 export default {
   components: {
@@ -44,8 +47,10 @@ export default {
   data() {
     return {
       commentLineStart: {
-        lineCode: this.line.line_code,
+        line_code: this.line.line_code,
         type: this.line.type,
+        old_line: this.line.old_line,
+        new_line: this.line.new_line,
       },
     };
   },
@@ -74,19 +79,26 @@ export default {
         diffViewType: this.diffViewType,
         diffFile: this.diffFile,
         linePosition: this.linePosition,
-        lineRange: {
-          start_line_code: this.commentLineStart.lineCode,
-          start_line_type: this.commentLineStart.type,
-          end_line_code: this.line.line_code,
-          end_line_type: this.line.type,
-        },
+        lineRange: formatLineRange(this.commentLineStart, this.line),
       };
     },
     diffFile() {
       return this.getDiffFileByHash(this.diffFileHash);
     },
     commentLineOptions() {
-      return commentLineOptions(this.diffFile.highlighted_diff_lines, this.line.line_code);
+      const combineSides = (acc, { left, right }) => {
+        // ignore null values match lines
+        if (left && left.type !== 'match') acc.push(left);
+        // if the line_codes are identically, return to avoid duplicates
+        if (left?.line_code === right?.line_code) return acc;
+        if (right && right.type !== 'match') acc.push(right);
+        return acc;
+      };
+      const side = this.line.type === 'new' ? 'right' : 'left';
+      const lines = this.diffFile.highlighted_diff_lines.length
+        ? this.diffFile.highlighted_diff_lines
+        : this.diffFile.parallel_diff_lines.reduce(combineSides, []);
+      return commentLineOptions(lines, this.line, this.line.line_code, side);
     },
   },
   mounted() {
@@ -136,10 +148,7 @@ export default {
 
 <template>
   <div class="content discussion-form discussion-form-container discussion-notes">
-    <div
-      v-if="glFeatures.multilineComments"
-      class="gl-mb-3 gl-text-gray-700 gl-border-gray-200 gl-border-b-solid gl-border-b-1 gl-pb-3"
-    >
+    <div v-if="glFeatures.multilineComments" class="gl-mb-3 gl-text-gray-700">
       <multiline-comment-form
         v-model="commentLineStart"
         :line="line"

app/assets/javascripts/notes/components/discussion_notes.vue

@@ -108,6 +108,7 @@ export default {
           :commit="commit"
           :help-page-path="helpPagePath"
           :show-reply-button="userCanReply"
+          :discussion-root="true"
           @handleDeleteNote="$emit('deleteNote')"
           @startReplying="$emit('startReplying')"
         >
@@ -151,6 +152,7 @@ export default {
           :note="componentData(note)"
           :help-page-path="helpPagePath"
           :line="diffLine"
+          :discussion-root="index === 0"
           @handleDeleteNote="$emit('deleteNote')"
         >
           <slot v-if="index === 0" slot="avatar-badge" name="avatar-badge"></slot>

app/assets/javascripts/notes/components/multiline_comment_form.vue

@@ -21,10 +21,23 @@ export default {
   },
   data() {
     return {
-      commentLineStart: {
-        lineCode: this.lineRange ? this.lineRange.start_line_code : this.line.line_code,
-        type: this.lineRange ? this.lineRange.start_line_type : this.line.type,
-      },
+      commentLineStart: {},
+      commentLineEndType: this.lineRange?.end?.line_type || this.line.type,
+    };
+  },
+  computed: {
+    lineNumber() {
+      return this.commentLineOptions[this.commentLineOptions.length - 1].text;
+    },
+  },
+  created() {
+    const line = this.lineRange?.start || this.line;
+
+    this.commentLineStart = {
+      line_code: line.line_code,
+      type: line.type,
+      old_line: line.old_line,
+      new_line: line.new_line,
     };
   },
   methods: {
@@ -34,6 +47,10 @@ export default {
     getLineClasses(line) {
       return getLineClasses(line);
     },
+    updateCommentLineStart(value) {
+      this.commentLineStart = value;
+      this.$emit('input', value);
+    },
   },
 };
 </script>
@@ -55,12 +72,12 @@ export default {
           :options="commentLineOptions"
           size="sm"
           class="gl-w-auto gl-vertical-align-baseline"
-          @change="$emit('input', $event)"
+          @change="updateCommentLineStart"
         />
       </template>
       <template #end>
         <span :class="getLineClasses(line)">
-          {{ getSymbol(line) + (line.new_line || line.old_line) }}
+          {{ lineNumber }}
         </span>
       </template>
     </gl-sprintf>

app/assets/javascripts/notes/components/multiline_comment_utils.js

@@ -7,11 +7,19 @@ export function getSymbol(type) {
 }
 
 function getLineNumber(lineRange, key) {
-  if (!lineRange || !key) return '';
-  const lineCode = lineRange[`${key}_line_code`] || '';
-  const lineType = lineRange[`${key}_line_type`] || '';
-  const lines = lineCode.split('_') || [];
-  const lineNumber = lineType === 'old' ? lines[1] : lines[2];
+  if (!lineRange || !key || !lineRange[key]) return '';
+  const { new_line: newLine, old_line: oldLine, type } = lineRange[key];
+  const otherKey = key === 'start' ? 'end' : 'start';
+
+  // By default we want to see the "old" or "left side" line number
+  // The exception is if the "end" line is on the "right" side
+  // `otherLineType` is only used if `type` is null to make sure the line
+  // number relfects the "right" side number, if that is the side
+  // the comment form is located on
+  const otherLineType = !type ? lineRange[otherKey]?.type : null;
+  const lineType = type || '';
+  let lineNumber = oldLine;
+  if (lineType === 'new' || otherLineType === 'new') lineNumber = newLine;
   return (lineNumber && getSymbol(lineType) + lineNumber) || '';
 }
 
@@ -37,21 +45,48 @@ export function getLineClasses(line) {
   ];
 }
 
-export function commentLineOptions(diffLines, lineCode) {
-  const selectedIndex = diffLines.findIndex(line => line.line_code === lineCode);
+export function commentLineOptions(diffLines, startingLine, lineCode, side = 'left') {
+  const preferredSide = side === 'left' ? 'old_line' : 'new_line';
+  const fallbackSide = preferredSide === 'new_line' ? 'old_line' : 'new_line';
   const notMatchType = l => l.type !== 'match';
+  const linesCopy = [...diffLines]; // don't mutate the argument
+  const startingLineCode = startingLine.line_code;
+
+  const currentIndex = linesCopy.findIndex(line => line.line_code === lineCode);
 
   // We're limiting adding comments to only lines above the current line
   // to make rendering simpler. Future interations will use a more
   // intuitive dragging interface that will make this unnecessary
-  const upToSelected = diffLines.slice(0, selectedIndex + 1);
+  const upToSelected = linesCopy.slice(0, currentIndex + 1);
 
   // Only include the lines up to the first "Show unchanged lines" block
   // i.e. not a "match" type
   const lines = takeRightWhile(upToSelected, notMatchType);
 
-  return lines.map(l => ({
-    value: { lineCode: l.line_code, type: l.type },
-    text: `${getSymbol(l.type)}${l.new_line || l.old_line}`,
-  }));
+  // If the selected line is "hidden" in an unchanged line block
+  // or "above" the current group of lines add it to the array so
+  // that the drop down is not defaulted to empty
+  const selectedIndex = lines.findIndex(line => line.line_code === startingLineCode);
+  if (selectedIndex < 0) lines.unshift(startingLine);
+
+  return lines.map(l => {
+    const { line_code, type, old_line, new_line } = l;
+    return {
+      value: { line_code, type, old_line, new_line },
+      text: `${getSymbol(type)}${l[preferredSide] || l[fallbackSide]}`,
+    };
+  });
+}
+
+export function formatLineRange(start, end) {
+  const extractProps = ({ line_code, type, old_line, new_line }) => ({
+    line_code,
+    type,
+    old_line,
+    new_line,
+  });
+  return {
+    start: extractProps(start),
+    end: extractProps(end),
+  };
 }

app/assets/javascripts/notes/components/noteable_note.vue

@@ -21,6 +21,7 @@ import {
   getEndLineNumber,
   getLineClasses,
   commentLineOptions,
+  formatLineRange,
 } from './multiline_comment_utils';
 import MultilineCommentForm from './multiline_comment_form.vue';
 
@@ -62,10 +63,15 @@ export default {
       default: false,
     },
     diffLines: {
-      type: Object,
+      type: Array,
       required: false,
       default: null,
     },
+    discussionRoot: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     return {
@@ -73,10 +79,7 @@ export default {
       isDeleting: false,
       isRequesting: false,
       isResolving: false,
-      commentLineStart: {
-        line_code: this.line?.line_code,
-        type: this.line?.type,
-      },
+      commentLineStart: {},
     };
   },
   computed: {
@@ -144,25 +147,42 @@ export default {
       return getEndLineNumber(this.lineRange);
     },
     showMultiLineComment() {
-      return (
-        this.glFeatures.multilineComments &&
-        this.startLineNumber &&
-        this.endLineNumber &&
-        (this.startLineNumber !== this.endLineNumber || this.isEditing)
-      );
+      if (!this.glFeatures.multilineComments) return false;
+      if (this.isEditing) return true;
+
+      return this.line && this.discussionRoot && this.startLineNumber !== this.endLineNumber;
     },
     commentLineOptions() {
-      if (this.diffLines) {
-        return commentLineOptions(this.diffLines, this.line.line_code);
+      if (!this.diffFile || !this.line) return [];
+
+      const sideA = this.line.type === 'new' ? 'right' : 'left';
+      const sideB = sideA === 'left' ? 'right' : 'left';
+      const lines = this.diffFile.highlighted_diff_lines.length
+        ? this.diffFile.highlighted_diff_lines
+        : this.diffFile.parallel_diff_lines.map(l => l[sideA] || l[sideB]);
+      return commentLineOptions(lines, this.commentLineStart, this.line.line_code, sideA);
+    },
+    diffFile() {
+      if (this.commentLineStart.line_code) {
+        const lineCode = this.commentLineStart.line_code.split('_')[0];
+        return this.getDiffFileByHash(lineCode);
       }
 
-      const diffFile = this.diffFile || this.getDiffFileByHash(this.targetNoteHash);
-      if (!diffFile) return null;
-      return commentLineOptions(diffFile.highlighted_diff_lines, this.line.line_code);
+      return null;
     },
   },
-
   created() {
+    const line = this.note.position?.line_range?.start || this.line;
+
+    this.commentLineStart = line
+      ? {
+          line_code: line.line_code,
+          type: line.type,
+          old_line: line.old_line,
+          new_line: line.new_line,
+        }
+      : {};
+
     eventHub.$on('enterEditMode', ({ noteId }) => {
       if (noteId === this.note.id) {
         this.isEditing = true;
@@ -224,13 +244,11 @@ export default {
     formUpdateHandler(noteText, parentElement, callback, resolveDiscussion) {
       const position = {
         ...this.note.position,
-        line_range: {
-          start_line_code: this.commentLineStart?.lineCode,
-          start_line_type: this.commentLineStart?.type,
-          end_line_code: this.line?.line_code,
-          end_line_type: this.line?.type,
-        },
       };
+
+      if (this.commentLineStart && this.line)
+        position.line_range = formatLineRange(this.commentLineStart, this.line);
+
       this.$emit('handleUpdateNote', {
         note: this.note,
         noteText,
@@ -246,7 +264,7 @@ export default {
         note: {
           target_type: this.getNoteableData.targetType,
           target_id: this.note.noteable_id,
-          note: { note: noteText },
+          note: { note: noteText, position: JSON.stringify(position) },
         },
       };
       this.isRequesting = true;
@@ -317,14 +335,17 @@ export default {
   >
     <div v-if="showMultiLineComment" data-testid="multiline-comment">
       <multiline-comment-form
-        v-if="isEditing && commentLineOptions && line"
+        v-if="isEditing && note.position"
         v-model="commentLineStart"
         :line="line"
         :comment-line-options="commentLineOptions"
         :line-range="note.position.line_range"
-        class="gl-mb-3 gl-text-gray-700 gl-border-gray-200 gl-border-b-solid gl-border-b-1 gl-pb-3"
+        class="gl-mb-3 gl-text-gray-700"
       />
-      <div v-else class="gl-mb-3 gl-text-gray-700">
+      <div
+        v-else
+        class="gl-mb-3 gl-text-gray-700 gl-border-gray-200 gl-border-b-solid gl-border-b-1 gl-pb-3"
+      >
         <gl-sprintf :message="__('Comment on lines %{startLine} to %{endLine}')">
           <template #startLine>
             <span :class="getLineClasses(startLineNumber)">{{ startLineNumber }}</span>

app/assets/javascripts/notes/mixins/diff_line_note_form.js

@@ -4,6 +4,7 @@ import { TEXT_DIFF_POSITION_TYPE, IMAGE_DIFF_POSITION_TYPE } from '~/diffs/const
 import createFlash from '~/flash';
 import { s__ } from '~/locale';
 import { clearDraft } from '~/lib/utils/autosave';
+import { formatLineRange } from '~/notes/components/multiline_comment_utils';
 
 export default {
   computed: {
@@ -45,6 +46,9 @@ export default {
         });
     },
     addToReview(note) {
+      const lineRange =
+        (this.line && this.commentLineStart && formatLineRange(this.commentLineStart, this.line)) ||
+        {};
       const positionType = this.diffFileCommentForm
         ? IMAGE_DIFF_POSITION_TYPE
         : TEXT_DIFF_POSITION_TYPE;
@@ -60,6 +64,7 @@ export default {
         linePosition: this.position,
         positionType,
         ...this.diffFileCommentForm,
+        lineRange,
       });
 
       const diffFileHeadSha = this.commit && this?.diffFile?.diff_refs?.head_sha;

app/controllers/concerns/notes_actions.rb

@@ -226,7 +226,7 @@ module NotesActions
   end
 
   def update_note_params
-    params.require(:note).permit(:note)
+    params.require(:note).permit(:note, :position)
   end
 
   def set_polling_interval_header

doc/api/discussions.md

@@ -782,10 +782,14 @@ Diff comments also contain position:
           "old_line": 27,
           "new_line": 27,
           "line_range": {
-            "start_line_code": "588440f66559714280628a4f9799f0c4eb880a4a_10_10",
-            "start_line_type": "new",
-            "end_line_code": "588440f66559714280628a4f9799f0c4eb880a4a_11_11",
-            "end_line_type": "old"
+            "start": {
+              "line_code": "588440f66559714280628a4f9799f0c4eb880a4a_10_10",
+              "type": "new",
+            },
+            "end": {
+              "line_code": "588440f66559714280628a4f9799f0c4eb880a4a_11_11",
+              "type": "old"
+            },
           }
         },
         "resolved": false,
@@ -832,30 +836,32 @@ POST /projects/:id/merge_requests/:merge_request_iid/discussions
 
 Parameters:
 
-| Attribute                               | Type           | Required | Description |
-| --------------------------------------- | -------------- | -------- | ----------- |
-| `id`                                    | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
-| `merge_request_iid`                     | integer        | yes      | The IID of a merge request |
-| `body`                                  | string         | yes      | The content of the thread |
-| `created_at`                            | string         | no       | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) |
-| `position`                              | hash           | no       | Position when creating a diff note |
-| `position[base_sha]`                    | string         | yes      | Base commit SHA in the source branch |
-| `position[start_sha]`                   | string         | yes      | SHA referencing commit in target branch |
-| `position[head_sha]`                    | string         | yes      | SHA referencing HEAD of this merge request |
-| `position[position_type]`               | string         | yes      | Type of the position reference', allowed values: 'text' or 'image' |
-| `position[new_path]`                    | string         | no       | File path after change |
-| `position[new_line]`                    | integer        | no       | Line number after change (for 'text' diff notes) |
-| `position[old_path]`                    | string         | no       | File path before change |
-| `position[old_line]`                    | integer        | no       | Line number before change (for 'text' diff notes) |
-| `position[line_range]`                  | hash           | no       | Line range for a multi-line diff note |
-| `position[line_range][start_line_code]` | string         | yes      | Line code for the start line |
-| `position[line_range][end_line_code]`   | string         | yes      | Line code for the end line |
-| `position[line_range][start_line_type]` | string         | yes      | Line type for the start line |
-| `position[line_range][end_line_type]`   | string         | yes      | Line type for the end line |
-| `position[width]`                       | integer        | no       | Width of the image (for 'image' diff notes) |
-| `position[height]`                      | integer        | no       | Height of the image (for 'image' diff notes) |
-| `position[x]`                           | integer        | no       | X coordinate (for 'image' diff notes) |
-| `position[y]`                           | integer        | no       | Y coordinate (for 'image' diff notes) |
+| Attribute                                | Type           | Required | Description |
+| ---------------------------------------- | -------------- | -------- | ----------- |
+| `id`                                     | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
+| `merge_request_iid`                      | integer        | yes      | The IID of a merge request |
+| `body`                                   | string         | yes      | The content of the thread |
+| `created_at`                             | string         | no       | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) |
+| `position`                               | hash           | no       | Position when creating a diff note |
+| `position[base_sha]`                     | string         | yes      | Base commit SHA in the source branch |
+| `position[start_sha]`                    | string         | yes      | SHA referencing commit in target branch |
+| `position[head_sha]`                     | string         | yes      | SHA referencing HEAD of this merge request |
+| `position[position_type]`                | string         | yes      | Type of the position reference', allowed values: 'text' or 'image' |
+| `position[new_path]`                     | string         | no       | File path after change |
+| `position[new_line]`                     | integer        | no       | Line number after change (for 'text' diff notes) |
+| `position[old_path]`                     | string         | no       | File path before change |
+| `position[old_line]`                     | integer        | no       | Line number before change (for 'text' diff notes) |
+| `position[line_range]`                   | hash           | no       | Line range for a multi-line diff note |
+| `position[line_range][start]`            | hash           | no       | Multiline note starting line |
+| `position[line_range][start][line_code]` | string         | yes      | Line code for the start line |
+| `position[line_range][start][type]`      | string         | yes      | Line type for the start line |
+| `position[line_range][end]`              | hash           | no       | Multiline note ending line |
+| `position[line_range][end][line_code]`   | string         | yes      | Line code for the end line |
+| `position[line_range][end][type]`        | string         | yes      | Line type for the end line |
+| `position[width]`                        | integer        | no       | Width of the image (for 'image' diff notes) |
+| `position[height]`                       | integer        | no       | Height of the image (for 'image' diff notes) |
+| `position[x]`                            | integer        | no       | X coordinate (for 'image' diff notes) |
+| `position[y]`                            | integer        | no       | Y coordinate (for 'image' diff notes) |
 
 ```shell
 curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions?body=comment"

lib/api/discussions.rb

@@ -76,10 +76,18 @@ module API
             optional :y, type: Integer, desc: 'Y coordinate in the image'
 
             optional :line_range, type: Hash, desc: 'Multi-line start and end' do
-              requires :start_line_code, type: String, desc: 'Start line code for multi-line note'
-              requires :end_line_code, type: String, desc: 'End line code for multi-line note'
-              requires :start_line_type, type: String, desc: 'Start line type for multi-line note'
-              requires :end_line_type, type: String, desc: 'End line type for multi-line note'
+              optional :start, type: Hash do
+                optional :line_code, type: String, desc: 'Start line code for multi-line note'
+                optional :type, type: String, desc: 'Start line type for multi-line note'
+                optional :old_line, type: String, desc: 'Start old_line line number'
+                optional :new_line, type: String, desc: 'Start new_line line number'
+              end
+              optional :end, type: Hash do
+                optional :line_code, type: String, desc: 'End line code for multi-line note'
+                optional :type, type: String, desc: 'End line type for multi-line note'
+                optional :old_line, type: String, desc: 'End old_line line number'
+                optional :new_line, type: String, desc: 'End new_line line number'
+              end
             end
           end
         end

spec/features/merge_request/user_comments_on_diff_spec.rb

@@ -117,9 +117,58 @@ RSpec.describe 'User comments on a diff', :js do
   context 'when adding multiline comments' do
     it 'saves a multiline comment' do
       click_diff_line(find("[id='#{sample_commit.line_code}']"))
+      add_comment('-13', '+14')
+    end
+
+    context 'when in side-by-side view' do
+      before do
+        visit(diffs_project_merge_request_path(project, merge_request, view: 'parallel'))
+      end
+
+      # In `files/ruby/popen.rb`
+      it 'allows comments for changes involving both sides' do
+        # click +15, select -13 add and verify comment
+        click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="15"]').find(:xpath, '../..'), 'right')
+        add_comment('-13', '+15')
+      end
+
+      it 'allows comments to start above hidden lines and end below' do
+        # click +28, select 21 add and verify comment
+        click_diff_line(find('div[data-path="files/ruby/popen.rb"] .new_line a[data-linenumber="28"]').find(:xpath, '../..'), 'right')
+        add_comment('21', '+28')
+      end
+
+      it 'allows comments on previously hidden lines at the top of a file' do
+        # Click -9, expand up, select 1 add and verify comment
+        page.within('[data-path="files/ruby/popen.rb"]') do
+          all('.js-unfold-all')[0].click
+        end
+        click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="9"]').find(:xpath, '../..'), 'left')
+        add_comment('1', '-9')
+      end
+
+      it 'allows comments on previously hidden lines the middle of a file' do
+        # Click 27, expand up, select 18, add and verify comment
+        page.within('[data-path="files/ruby/popen.rb"]') do
+          all('.js-unfold-all')[1].click
+        end
+        click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="21"]').find(:xpath, '../..'), 'left')
+        add_comment('18', '21')
+      end
+
+      it 'allows comments on previously hidden lines at the bottom of a file' do
+        # Click +28, expand down, select 37 add and verify comment
+        page.within('[data-path="files/ruby/popen.rb"]') do
+          all('.js-unfold-down')[1].click
+        end
+        click_diff_line(find('div[data-path="files/ruby/popen.rb"] .old_line a[data-linenumber="30"]').find(:xpath, '../..'), 'left')
+        add_comment('+28', '37')
+      end
+    end
 
+    def add_comment(start_line, end_line)
       page.within('.discussion-form') do
-        find('#comment-line-start option', text: '-13').select_option
+        find('#comment-line-start option', exact_text: start_line).select_option
       end
 
       page.within('.js-discussion-note-form') do
@@ -131,7 +180,7 @@ RSpec.describe 'User comments on a diff', :js do
 
       page.within('.notes_holder') do
         expect(page).to have_content('Line is wrong')
-        expect(page).to have_content('Comment on lines -13 to +14')
+        expect(page).to have_content("Comment on lines #{start_line} to #{end_line}")
       end
 
       visit(merge_request_path(merge_request))

spec/frontend/diffs/components/diff_line_note_form_spec.js

@@ -78,10 +78,18 @@ describe('DiffLineNoteForm', () => {
           .mockReturnValue(Promise.resolve());
 
         const lineRange = {
-          start_line_code: wrapper.vm.commentLineStart.lineCode,
-          start_line_type: wrapper.vm.commentLineStart.type,
-          end_line_code: wrapper.vm.line.line_code,
-          end_line_type: wrapper.vm.line.type,
+          start: {
+            line_code: wrapper.vm.commentLineStart.line_code,
+            type: wrapper.vm.commentLineStart.type,
+            new_line: 1,
+            old_line: null,
+          },
+          end: {
+            line_code: wrapper.vm.line.line_code,
+            type: wrapper.vm.line.type,
+            new_line: 1,
+            old_line: null,
+          },
         };
 
         const formData = {

spec/frontend/notes/components/multiline_comment_utils_spec.js

@@ -5,32 +5,19 @@ import {
 } from '~/notes/components/multiline_comment_utils';
 
 describe('Multiline comment utilities', () => {
-  describe('getStartLineNumber', () => {
+  describe('get start & end line numbers', () => {
+    const lineRanges = ['old', 'new', null].map(type => ({
+      start: { new_line: 1, old_line: 1, type },
+      end: { new_line: 2, old_line: 2, type },
+    }));
     it.each`
-      lineCode        | type     | result
-      ${'abcdef_1_1'} | ${'old'} | ${'-1'}
-      ${'abcdef_1_1'} | ${'new'} | ${'+1'}
-      ${'abcdef_1_1'} | ${null}  | ${'1'}
-      ${'abcdef'}     | ${'new'} | ${''}
-      ${'abcdef'}     | ${'old'} | ${''}
-      ${'abcdef'}     | ${null}  | ${''}
-    `('returns line number', ({ lineCode, type, result }) => {
-      expect(getStartLineNumber({ start_line_code: lineCode, start_line_type: type })).toEqual(
-        result,
-      );
-    });
-  });
-  describe('getEndLineNumber', () => {
-    it.each`
-      lineCode        | type     | result
-      ${'abcdef_1_1'} | ${'old'} | ${'-1'}
-      ${'abcdef_1_1'} | ${'new'} | ${'+1'}
-      ${'abcdef_1_1'} | ${null}  | ${'1'}
-      ${'abcdef'}     | ${'new'} | ${''}
-      ${'abcdef'}     | ${'old'} | ${''}
-      ${'abcdef'}     | ${null}  | ${''}
-    `('returns line number', ({ lineCode, type, result }) => {
-      expect(getEndLineNumber({ end_line_code: lineCode, end_line_type: type })).toEqual(result);
+      lineRange        | start   | end
+      ${lineRanges[0]} | ${'-1'} | ${'-2'}
+      ${lineRanges[1]} | ${'+1'} | ${'+2'}
+      ${lineRanges[2]} | ${'1'}  | ${'2'}
+    `('returns line numbers `$start` & `$end`', ({ lineRange, start, end }) => {
+      expect(getStartLineNumber(lineRange)).toEqual(start);
+      expect(getEndLineNumber(lineRange)).toEqual(end);
     });
   });
   describe('getSymbol', () => {

spec/frontend/notes/components/noteable_note_spec.js

@@ -46,12 +46,30 @@ describe('issue_note', () => {
     it('should render if has multiline comment', () => {
       const position = {
         line_range: {
-          start_line_code: 'abc_1_1',
-          end_line_code: 'abc_2_2',
+          start: {
+            line_code: 'abc_1_1',
+            type: null,
+            old_line: '1',
+            new_line: '1',
+          },
+          end: {
+            line_code: 'abc_2_2',
+            type: null,
+            old_line: '2',
+            new_line: '2',
+          },
         },
       };
+      const line = {
+        line_code: 'abc_1_1',
+        type: null,
+        old_line: '1',
+        new_line: '1',
+      };
       wrapper.setProps({
         note: { ...note, position },
+        discussionRoot: true,
+        line,
       });
 
       return wrapper.vm.$nextTick().then(() => {
@@ -62,12 +80,30 @@ describe('issue_note', () => {
     it('should not render if has single line comment', () => {
       const position = {
         line_range: {
-          start_line_code: 'abc_1_1',
-          end_line_code: 'abc_1_1',
+          start: {
+            line_code: 'abc_1_1',
+            type: null,
+            old_line: '1',
+            new_line: '1',
+          },
+          end: {
+            line_code: 'abc_1_1',
+            type: null,
+            old_line: '1',
+            new_line: '1',
+          },
         },
       };
+      const line = {
+        line_code: 'abc_1_1',
+        type: null,
+        old_line: '1',
+        new_line: '1',
+      };
       wrapper.setProps({
         note: { ...note, position },
+        discussionRoot: true,
+        line,
       });
 
       return wrapper.vm.$nextTick().then(() => {