Update diffs interop API

Now we expose data- attributes
which can be picked up by third parties
such as Sourcegraph. This is much more
reliable than coupling to our ever changing
HTML structure.

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

@@ -13,6 +13,11 @@ import {
   CONFLICT_THEIR,
   CONFLICT_MARKER,
 } from '../constants';
+import {
+  getInteropInlineAttributes,
+  getInteropOldSideAttributes,
+  getInteropNewSideAttributes,
+} from '../utils/interoperability';
 import DiffGutterAvatars from './diff_gutter_avatars.vue';
 import * as utils from './diff_row_utils';
 
@@ -116,6 +121,16 @@ export default {
     isLeftConflictMarker() {
       return [CONFLICT_MARKER_OUR, CONFLICT_MARKER_THEIR].includes(this.line.left?.type);
     },
+    interopLeftAttributes() {
+      if (this.inline) {
+        return getInteropInlineAttributes(this.line.left);
+      }
+
+      return getInteropOldSideAttributes(this.line.left);
+    },
+    interopRightAttributes() {
+      return getInteropNewSideAttributes(this.line.right);
+    },
   },
   mounted() {
     this.scrollToLineIfNeededParallel(this.line);
@@ -181,6 +196,7 @@ export default {
     <div
       data-testid="left-side"
       class="diff-grid-left left-side"
+      v-bind="interopLeftAttributes"
       @dragover.prevent
       @dragenter="onDragEnter(line.left, index)"
       @dragend="onDragEnd"
@@ -286,6 +302,7 @@ export default {
       v-if="!inline"
       data-testid="right-side"
       class="diff-grid-right right-side"
+      v-bind="interopRightAttributes"
       @dragover.prevent
       @dragenter="onDragEnter(line.right, index)"
       @dragend="onDragEnd"

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

@@ -2,6 +2,7 @@
 import { GlTooltipDirective, GlIcon, GlSafeHtmlDirective as SafeHtml } from '@gitlab/ui';
 import { mapActions, mapGetters, mapState } from 'vuex';
 import { CONTEXT_LINE_CLASS_NAME } from '../constants';
+import { getInteropInlineAttributes } from '../utils/interoperability';
 import DiffGutterAvatars from './diff_gutter_avatars.vue';
 import {
   isHighlighted,
@@ -96,6 +97,9 @@ export default {
     shouldShowAvatarsOnGutter() {
       return this.line.hasDiscussions;
     },
+    interopAttrs() {
+      return getInteropInlineAttributes(this.line);
+    },
   },
   mounted() {
     this.scrollToLineIfNeededInline(this.line);
@@ -124,6 +128,7 @@ export default {
     :id="inlineRowId"
     :class="classNameMap"
     class="line_holder"
+    v-bind="interopAttrs"
     @mouseover="handleMouseMove"
     @mouseout="handleMouseMove"
   >

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

@@ -3,6 +3,10 @@ import { GlTooltipDirective, GlIcon, GlSafeHtmlDirective as SafeHtml } from '@gi
 import $ from 'jquery';
 import { mapActions, mapGetters, mapState } from 'vuex';
 import { CONTEXT_LINE_CLASS_NAME, PARALLEL_DIFF_VIEW_TYPE } from '../constants';
+import {
+  getInteropOldSideAttributes,
+  getInteropNewSideAttributes,
+} from '../utils/interoperability';
 import DiffGutterAvatars from './diff_gutter_avatars.vue';
 import * as utils from './diff_row_utils';
 
@@ -108,6 +112,12 @@ export default {
         this.line.hasDiscussionsRight,
       );
     },
+    interopLeftAttributes() {
+      return getInteropOldSideAttributes(this.line.left);
+    },
+    interopRightAttributes() {
+      return getInteropNewSideAttributes(this.line.right);
+    },
   },
   mounted() {
     this.scrollToLineIfNeededParallel(this.line);
@@ -217,6 +227,7 @@ export default {
         :key="line.left.line_code"
         v-safe-html="line.left.rich_text"
         :class="parallelViewLeftLineType"
+        v-bind="interopLeftAttributes"
         class="line_content with-coverage parallel left-side"
         @mousedown="handleParallelLineMouseDown"
       ></td>
@@ -283,6 +294,7 @@ export default {
             hll: isHighlighted,
           },
         ]"
+        v-bind="interopRightAttributes"
         class="line_content with-coverage parallel right-side"
         @mousedown="handleParallelLineMouseDown"
       ></td>

app/assets/javascripts/diffs/utils/interoperability.js

+const OLD = 'old';
+const NEW = 'new';
+const ATTR_PREFIX = 'data-interop-';
+
+export const ATTR_TYPE = `${ATTR_PREFIX}type`;
+export const ATTR_LINE = `${ATTR_PREFIX}line`;
+export const ATTR_NEW_LINE = `${ATTR_PREFIX}new-line`;
+export const ATTR_OLD_LINE = `${ATTR_PREFIX}old-line`;
+
+export const getInteropInlineAttributes = (line) => {
+  if (!line) {
+    return null;
+  }
+
+  const interopType = line.type?.startsWith(OLD) ? OLD : NEW;
+
+  const interopLine = interopType === OLD ? line.old_line : line.new_line;
+
+  return {
+    [ATTR_TYPE]: interopType,
+    [ATTR_LINE]: interopLine,
+    [ATTR_NEW_LINE]: line.new_line,
+    [ATTR_OLD_LINE]: line.old_line,
+  };
+};
+
+export const getInteropOldSideAttributes = (line) => {
+  if (!line) {
+    return null;
+  }
+
+  return {
+    [ATTR_TYPE]: OLD,
+    [ATTR_LINE]: line.old_line,
+    [ATTR_OLD_LINE]: line.old_line,
+  };
+};
+
+export const getInteropNewSideAttributes = (line) => {
+  if (!line) {
+    return null;
+  }
+
+  return {
+    [ATTR_TYPE]: NEW,
+    [ATTR_LINE]: line.new_line,
+    [ATTR_NEW_LINE]: line.new_line,
+  };
+};

spec/frontend/diffs/components/diff_row_spec.js

@@ -4,6 +4,7 @@ import Vuex from 'vuex';
 import DiffRow from '~/diffs/components/diff_row.vue';
 import { mapParallel } from '~/diffs/components/diff_row_utils';
 import diffsModule from '~/diffs/store/modules';
+import { findInteropAttributes } from '../find_interop_attributes';
 import diffFileMockData from '../mock_data/diff_file';
 
 describe('DiffRow', () => {
@@ -211,4 +212,20 @@ describe('DiffRow', () => {
       expect(coverage.classes('no-coverage')).toBeFalsy();
     });
   });
+
+  describe('interoperability', () => {
+    it.each`
+      desc                                 | line                                                   | inline   | leftSide                                                  | rightSide
+      ${'with inline and new_line'}        | ${{ left: { old_line: 3, new_line: 5, type: 'new' } }} | ${true}  | ${{ type: 'new', line: '5', oldLine: '3', newLine: '5' }} | ${null}
+      ${'with inline and no new_line'}     | ${{ left: { old_line: 3, type: 'old' } }}              | ${true}  | ${{ type: 'old', line: '3', oldLine: '3' }}               | ${null}
+      ${'with parallel and no right side'} | ${{ left: { old_line: 3, new_line: 5 } }}              | ${false} | ${{ type: 'old', line: '3', oldLine: '3' }}               | ${null}
+      ${'with parallel and no left side'}  | ${{ right: { old_line: 3, new_line: 5 } }}             | ${false} | ${null}                                                   | ${{ type: 'new', line: '5', newLine: '5' }}
+      ${'with parallel and right side'}    | ${{ left: { old_line: 3 }, right: { new_line: 5 } }}   | ${false} | ${{ type: 'old', line: '3', oldLine: '3' }}               | ${{ type: 'new', line: '5', newLine: '5' }}
+    `('$desc, sets interop data attributes', ({ line, inline, leftSide, rightSide }) => {
+      const wrapper = createWrapper({ props: { line, inline } });
+
+      expect(findInteropAttributes(wrapper, '[data-testid="left-side"]')).toEqual(leftSide);
+      expect(findInteropAttributes(wrapper, '[data-testid="right-side"]')).toEqual(rightSide);
+    });
+  });
 });

spec/frontend/diffs/components/inline_diff_table_row_spec.js

@@ -3,6 +3,7 @@ import DiffGutterAvatars from '~/diffs/components/diff_gutter_avatars.vue';
 import { mapInline } from '~/diffs/components/diff_row_utils';
 import InlineDiffTableRow from '~/diffs/components/inline_diff_table_row.vue';
 import { createStore } from '~/mr_notes/stores';
+import { findInteropAttributes } from '../find_interop_attributes';
 import discussionsMockData from '../mock_data/diff_discussions';
 import diffFileMockData from '../mock_data/diff_file';
 
@@ -310,4 +311,16 @@ describe('InlineDiffTableRow', () => {
       });
     });
   });
+
+  describe('interoperability', () => {
+    it.each`
+      desc               | line                                                      | expectation
+      ${'with type old'} | ${{ ...thisLine, type: 'old', old_line: 3, new_line: 5 }} | ${{ type: 'old', line: '3', oldLine: '3', newLine: '5' }}
+      ${'with type new'} | ${{ ...thisLine, type: 'new', old_line: 3, new_line: 5 }} | ${{ type: 'new', line: '5', oldLine: '3', newLine: '5' }}
+    `('$desc, sets interop data attributes', ({ line, expectation }) => {
+      createComponent({ line });
+
+      expect(findInteropAttributes(wrapper)).toEqual(expectation);
+    });
+  });
 });

spec/frontend/diffs/components/parallel_diff_table_row_spec.js

@@ -5,6 +5,7 @@ import DiffGutterAvatars from '~/diffs/components/diff_gutter_avatars.vue';
 import { mapParallel } from '~/diffs/components/diff_row_utils';
 import ParallelDiffTableRow from '~/diffs/components/parallel_diff_table_row.vue';
 import { createStore } from '~/mr_notes/stores';
+import { findInteropAttributes } from '../find_interop_attributes';
 import discussionsMockData from '../mock_data/diff_discussions';
 import diffFileMockData from '../mock_data/diff_file';
 
@@ -418,5 +419,27 @@ describe('ParallelDiffTableRow', () => {
         });
       });
     });
+
+    describe('interoperability', () => {
+      beforeEach(() => {
+        createComponent();
+      });
+
+      it('adds old side interoperability data attributes', () => {
+        expect(findInteropAttributes(wrapper, '.line_content.left-side')).toEqual({
+          type: 'old',
+          line: thisLine.left.old_line.toString(),
+          oldLine: thisLine.left.old_line.toString(),
+        });
+      });
+
+      it('adds new side interoperability data attributes', () => {
+        expect(findInteropAttributes(wrapper, '.line_content.right-side')).toEqual({
+          type: 'new',
+          line: thisLine.right.new_line.toString(),
+          newLine: thisLine.right.new_line.toString(),
+        });
+      });
+    });
   });
 });

spec/frontend/diffs/find_interop_attributes.js

+export const findInteropAttributes = (parent, sel) => {
+  const target = sel ? parent.find(sel) : parent;
+
+  if (!target.exists()) {
+    return null;
+  }
+
+  const type = target.attributes('data-interop-type');
+
+  if (!type) {
+    return null;
+  }
+
+  return {
+    type,
+    line: target.attributes('data-interop-line'),
+    oldLine: target.attributes('data-interop-old-line'),
+    newLine: target.attributes('data-interop-new-line'),
+  };
+};

spec/frontend/diffs/utils/interoperability_spec.js

+import {
+  getInteropInlineAttributes,
+  getInteropNewSideAttributes,
+  getInteropOldSideAttributes,
+  ATTR_TYPE,
+  ATTR_LINE,
+  ATTR_NEW_LINE,
+  ATTR_OLD_LINE,
+} from '~/diffs/utils/interoperability';
+
+describe('~/diffs/utils/interoperability', () => {
+  describe('getInteropInlineAttributes', () => {
+    it.each([
+      ['with null input', { input: null, output: null }],
+      [
+        'with type=old input',
+        {
+          input: { type: 'old', old_line: 3, new_line: 5 },
+          output: { [ATTR_TYPE]: 'old', [ATTR_LINE]: 3, [ATTR_OLD_LINE]: 3, [ATTR_NEW_LINE]: 5 },
+        },
+      ],
+      [
+        'with type=old-nonewline input',
+        {
+          input: { type: 'old-nonewline', old_line: 3, new_line: 5 },
+          output: { [ATTR_TYPE]: 'old', [ATTR_LINE]: 3, [ATTR_OLD_LINE]: 3, [ATTR_NEW_LINE]: 5 },
+        },
+      ],
+      [
+        'with type=new input',
+        {
+          input: { type: 'new', old_line: 3, new_line: 5 },
+          output: { [ATTR_TYPE]: 'new', [ATTR_LINE]: 5, [ATTR_OLD_LINE]: 3, [ATTR_NEW_LINE]: 5 },
+        },
+      ],
+      [
+        'with type=bogus input',
+        {
+          input: { type: 'bogus', old_line: 3, new_line: 5 },
+          output: { [ATTR_TYPE]: 'new', [ATTR_LINE]: 5, [ATTR_OLD_LINE]: 3, [ATTR_NEW_LINE]: 5 },
+        },
+      ],
+    ])('%s', (desc, { input, output }) => {
+      expect(getInteropInlineAttributes(input)).toEqual(output);
+    });
+  });
+
+  describe('getInteropOldSideAttributes', () => {
+    it.each`
+      input              | output
+      ${null}            | ${null}
+      ${{ old_line: 2 }} | ${{ [ATTR_TYPE]: 'old', [ATTR_LINE]: 2, [ATTR_OLD_LINE]: 2 }}
+    `('with input=$input', ({ input, output }) => {
+      expect(getInteropOldSideAttributes(input)).toEqual(output);
+    });
+  });
+
+  describe('getInteropNewSideAttributes', () => {
+    it.each`
+      input              | output
+      ${null}            | ${null}
+      ${{ new_line: 2 }} | ${{ [ATTR_TYPE]: 'new', [ATTR_LINE]: 2, [ATTR_NEW_LINE]: 2 }}
+    `('with input=$input', ({ input, output }) => {
+      expect(getInteropNewSideAttributes(input)).toEqual(output);
+    });
+  });
+});

spec/frontend_integration/diffs/diffs_interopability_api.js

 /**
- * This helper module helps freeze the API expectation of the diff output.
+ * This helper module contains the API expectation of the diff output HTML.
  *
- * This helps simulate what third-parties, such as Sourcegraph, which scrape
- * the HTML shold be looking fo.
- *
- * TEMPORARY! These functions are copied from Sourcegraph
+ * This helps simulate what third-party HTML scrapers, such as Sourcegraph,
+ * should be looking for.
  */
 export const getDiffCodePart = (codeElement) => {
-  let selector = 'old';
-
-  const row = codeElement.closest('.diff-td,td');
+  const el = codeElement.closest('[data-interop-type]');
 
-  // Split diff
-  if (row.classList.contains('parallel')) {
-    selector = 'left-side';
-  }
-
-  return row.classList.contains(selector) ? 'base' : 'head';
+  return el.dataset.interopType === 'old' ? 'base' : 'head';
 };
 
 export const getCodeElementFromLineNumber = (codeView, line, part) => {
-  const lineNumberElement = codeView.querySelector(
-    `.${part === 'base' ? 'old_line' : 'new_line'} [data-linenumber="${line}"]`,
-  );
-  if (!lineNumberElement) {
-    return null;
-  }
-
-  const row = lineNumberElement.closest('.diff-tr,tr');
-  if (!row) {
-    return null;
-  }
+  const type = part === 'base' ? 'old' : 'new';
 
-  let selector = 'span.line';
+  const el = codeView.querySelector(`[data-interop-${type}-line="${line}"]`);
 
-  // Split diff
-  if (row.classList.contains('parallel')) {
-    selector = `.${part === 'base' ? 'left-side' : 'right-side'} ${selector}`;
-  }
-
-  return row.querySelector(selector);
+  return el ? el.querySelector('span.line') : null;
 };
 
-export const getLineNumberFromCodeElement = (el) => {
-  const part = getDiffCodePart(el);
-
-  let cell = el.closest('.diff-td,td');
-  while (
-    cell &&
-    !cell.matches(`.diff-line-num.${part === 'base' ? 'old_line' : 'new_line'}`) &&
-    cell.previousElementSibling
-  ) {
-    cell = cell.previousElementSibling;
-  }
-
-  if (cell) {
-    const a = cell.querySelector('a');
-    return parseInt(a.dataset.linenumber || '', 10);
-  }
+export const getLineNumberFromCodeElement = (codeElement) => {
+  const el = codeElement.closest('[data-interop-line]');
 
-  throw new Error('Unable to determine line number for diff code element');
+  return parseInt(el.dataset.interopLine || '', 10);
 };

spec/frontend_integration/diffs/diffs_interopability_spec.js

@@ -2,7 +2,11 @@ import { waitFor } from '@testing-library/dom';
 import { TEST_HOST } from 'helpers/test_constants';
 import initDiffsApp from '~/diffs';
 import { createStore } from '~/mr_notes/stores';
-import { getDiffCodePart, getLineNumberFromCodeElement } from './diffs_interopability_api';
+import {
+  getDiffCodePart,
+  getLineNumberFromCodeElement,
+  getCodeElementFromLineNumber,
+} from './diffs_interopability_api';
 
 jest.mock('~/vue_shared/mixins/gl_feature_flags_mixin', () => () => ({
   inject: {
@@ -20,7 +24,7 @@ const EXPECT_INLINE = [
   ['head', 1],
   ['head', 2],
   ['head', 3],
-  ['base', 5],
+  ['base', 4],
   ['head', 4],
   null,
   ['base', 6],
@@ -109,13 +113,6 @@ describe('diffs third party interoperability', () => {
         ],
     );
 
-  // ${'inline view'}              | ${false}              | ${'inline'}   | ${'tr.line_holder'}       | ${'td.line_content'}                  | ${EXPECT_INLINE}
-  // ${'parallel view left side'}  | ${false}              | ${'parallel'} | ${'tr.line_holder'}       | ${'td.line_content.left-side'}        | ${EXPECT_PARALLEL_LEFT_SIDE}
-  // ${'parallel view right side'} | ${false}              | ${'parallel'} | ${'tr.line_holder'}       | ${'td.line_content.right-side'}       | ${EXPECT_PARALLEL_RIGHT_SIDE}
-  // ${'inline view'}              | ${true}               | ${'inline'}   | ${'.diff-tr.line_holder'} | ${'.diff-td.line_content'}            | ${EXPECT_INLINE}
-  // ${'parallel view left side'}  | ${true}               | ${'parallel'} | ${'.diff-tr.line_holder'} | ${'.diff-td.line_content.left-side'}  | ${EXPECT_PARALLEL_LEFT_SIDE.slice(0, EXPECT_PARALLEL_LEFT_SIDE.length - 1)}
-  // ${'parallel view right side'} | ${true}               | ${'parallel'} | ${'.diff-tr.line_holder'} | ${'.diff-td.line_content.right-side'} | ${EXPECT_PARALLEL_RIGHT_SIDE.slice(0, EXPECT_PARALLEL_RIGHT_SIDE.length - 1)}
-
   describe.each`
     desc                          | unifiedDiffComponents | view          | rowSelector               | codeSelector                          | expectation
     ${'inline view'}              | ${false}              | ${'inline'}   | ${'tr.line_holder'}       | ${'td.line_content'}                  | ${EXPECT_INLINE}
@@ -144,6 +141,21 @@ describe('diffs third party interoperability', () => {
 
         expect(getCodeElementsInteropModel(codes)).toEqual(expectation);
       });
+
+      it.each`
+        lineNumber | part      | expectedText
+        ${4}       | ${'base'} | ${'new CommitFile(this)'}
+        ${4}       | ${'head'} | ${'new CommitFile(@)'}
+        ${2}       | ${'base'} | ${'constructor: ->'}
+        ${2}       | ${'head'} | ${'constructor: ->'}
+      `(
+        'should find code element lineNumber=$lineNumber part=$part',
+        ({ lineNumber, part, expectedText }) => {
+          const codeElement = getCodeElementFromLineNumber(findDiffFile(), lineNumber, part);
+
+          expect(codeElement.textContent.trim()).toBe(expectedText);
+        },
+      );
     },
   );
 });