Commit 61b90085 authored by Phil Hughes's avatar Phil Hughes

Converts the diff row component into a functional component

This converts the diff row component into a functional component
to help increase the rendering performance and memory usage
off the diffs app.

Changelog: added
parent 0e70e292
......@@ -6,13 +6,17 @@ import {
OLD_NO_NEW_LINE_TYPE,
NEW_NO_NEW_LINE_TYPE,
EMPTY_CELL_TYPE,
CONFLICT_MARKER_OUR,
CONFLICT_MARKER_THEIR,
CONFLICT_THEIR,
CONFLICT_OUR,
} from '../constants';
export const isHighlighted = (state, line, isCommented) => {
export const isHighlighted = (highlightedRow, line, isCommented) => {
if (isCommented) return true;
const lineCode = line?.line_code;
return lineCode ? lineCode === state.diffs.highlightedRow : false;
return lineCode ? lineCode === highlightedRow : false;
};
export const isContextLine = (type) => type === CONTEXT_LINE_TYPE;
......@@ -50,13 +54,11 @@ export const classNameMapCell = ({ line, hll, isLoggedIn, isHover }) => {
];
};
export const addCommentTooltip = (line, dragCommentSelectionEnabled = false) => {
export const addCommentTooltip = (line) => {
let tooltip;
if (!line) return tooltip;
tooltip = dragCommentSelectionEnabled
? __('Add a comment to this line or drag for multiple lines')
: __('Add a comment to this line');
tooltip = __('Add a comment to this line or drag for multiple lines');
const brokenSymlinks = line.commentsDisabled;
if (brokenSymlinks) {
......@@ -107,6 +109,10 @@ export const mapParallel = (content) => (line) => {
hasDraft: content.hasParallelDraftLeft(content.diffFile.file_hash, line),
lineDraft: content.draftForLine(content.diffFile.file_hash, line, 'left'),
hasCommentForm: left.hasForm,
isConflictMarker:
line.left.type === CONFLICT_MARKER_OUR || line.left.type === CONFLICT_MARKER_THEIR,
emptyCellClassMap: { conflict_our: line.right?.type === CONFLICT_THEIR },
addCommentTooltip: addCommentTooltip(line.left),
};
}
if (right) {
......@@ -116,6 +122,8 @@ export const mapParallel = (content) => (line) => {
hasDraft: content.hasParallelDraftRight(content.diffFile.file_hash, line),
lineDraft: content.draftForLine(content.diffFile.file_hash, line, 'right'),
hasCommentForm: Boolean(right.hasForm && right.type),
emptyCellClassMap: { conflict_their: line.left?.type === CONFLICT_OUR },
addCommentTooltip: addCommentTooltip(line.right),
};
}
......
......@@ -3,10 +3,12 @@ import { mapGetters, mapState, mapActions } from 'vuex';
import DraftNote from '~/batch_comments/components/draft_note.vue';
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 DiffCommentCell from './diff_comment_cell.vue';
import DiffExpansionCell from './diff_expansion_cell.vue';
import DiffRow from './diff_row.vue';
import { isHighlighted } from './diff_row_utils';
export default {
components: {
......@@ -43,8 +45,8 @@ export default {
};
},
computed: {
...mapGetters('diffs', ['commitId']),
...mapState('diffs', ['codequalityDiff']),
...mapGetters('diffs', ['commitId', 'fileLineCoverage']),
...mapState('diffs', ['codequalityDiff', 'highlightedRow']),
...mapState({
selectedCommentPosition: ({ notes }) => notes.selectedCommentPosition,
selectedCommentPositionHover: ({ notes }) => notes.selectedCommentPositionHover,
......@@ -67,14 +69,17 @@ export default {
},
methods: {
...mapActions(['setSelectedCommentPosition']),
...mapActions('diffs', ['showCommentForm']),
...mapActions('diffs', ['showCommentForm', 'setHighlightedRow', 'toggleLineDiscussions']),
showCommentLeft(line) {
return line.left && !line.right;
},
showCommentRight(line) {
return line.right && !line.left;
},
onStartDragging(line) {
onStartDragging({ event = {}, line }) {
if (event.target?.parentNode) {
hide(event.target.parentNode);
}
this.dragStart = line;
},
onDragOver(line) {
......@@ -99,6 +104,26 @@ export default {
});
this.dragStart = null;
},
isHighlighted(line) {
return isHighlighted(
this.highlightedRow,
line.left?.line_code ? line.left : line.right,
false,
);
},
handleParallelLineMouseDown(e) {
const line = e.target.closest('.diff-td');
const table = line.closest('.diff-table');
table.classList.remove('left-side-selected', 'right-side-selected');
const [lineClass] = ['left-side', 'right-side'].filter((name) =>
line.classList.contains(name),
);
if (lineClass) {
table.classList.add(`${lineClass}-selected`);
}
},
},
userColorScheme: window.gon.user_color_scheme,
};
......@@ -109,6 +134,7 @@ export default {
:class="[$options.userColorScheme, { inline, 'with-codequality': hasCodequalityChanges }]"
:data-commit-id="commitId"
class="diff-grid diff-table code diff-wrap-lines js-syntax-highlight text-file"
@mousedown="handleParallelLineMouseDown"
>
<template v-for="(line, index) in diffLines">
<div
......@@ -136,6 +162,14 @@ export default {
:is-commented="index >= commentedLines.startLine && index <= commentedLines.endLine"
:inline="inline"
:index="index"
:is-highlighted="isHighlighted(line)"
:file-line-coverage="fileLineCoverage"
@showCommentForm="(lineCode) => showCommentForm({ lineCode, fileHash: diffFile.file_hash })"
@setHighlightedRow="setHighlightedRow"
@toggleLineDiscussions="
({ lineCode, expanded }) =>
toggleLineDiscussions({ lineCode, fileHash: diffFile.file_hash, expanded })
"
@enterdragging="onDragOver"
@startdragging="onStartDragging"
@stopdragging="onStopDragging"
......
......@@ -235,6 +235,8 @@ export const setHighlightedRow = ({ commit }, lineCode) => {
const fileHash = lineCode.split('_')[0];
commit(types.SET_HIGHLIGHTED_ROW, lineCode);
commit(types.VIEW_DIFF_FILE, fileHash);
handleLocationHash();
};
// This is adding line discussions to the actual lines in the diff tree
......
......@@ -763,6 +763,7 @@ $system-note-svg-size: 16px;
.note-button.add-diff-note {
@include btn-comment-icon;
opacity: 0;
will-change: opacity;
&[disabled] {
background: $white;
......
......@@ -32,7 +32,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
push_frontend_feature_flag(:file_identifier_hash)
push_frontend_feature_flag(:approvals_commented_by, @project, default_enabled: true)
push_frontend_feature_flag(:merge_request_widget_graphql, @project, default_enabled: :yaml)
push_frontend_feature_flag(:drag_comment_selection, @project, default_enabled: true)
push_frontend_feature_flag(:default_merge_ref_for_diffs, @project, default_enabled: :yaml)
push_frontend_feature_flag(:core_security_mr_widget_counts, @project)
push_frontend_feature_flag(:local_file_reviews, default_enabled: :yaml)
......
---
name: drag_comment_selection
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49875
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/293945
milestone: '13.7'
type: development
group: group::source code
default_enabled: true
import { shallowMount } from '@vue/test-utils';
import Vue from 'vue';
import Vuex from 'vuex';
import CodeQualityGutterIcon from 'ee/diffs/components/code_quality_gutter_icon.vue';
import DiffRow from '~/diffs/components/diff_row.vue';
import diffsModule from '~/diffs/store/modules';
......@@ -10,16 +9,24 @@ Vue.use(Vuex);
describe('EE DiffRow', () => {
let wrapper;
const findIcon = () => wrapper.findComponent(CodeQualityGutterIcon);
const findIcon = () => wrapper.find('[data-testid="codeQualityIcon"]');
const defaultProps = {
fileHash: 'abc',
filePath: 'abc',
line: {},
index: 0,
isHighlighted: false,
fileLineCoverage: () => ({}),
};
const createComponent = ({ props, state, actions, isLoggedIn = true, provide }) => {
const createComponent = ({
props,
state,
actions,
isLoggedIn = true,
codequalityMrDiffAnnotations,
}) => {
const diffs = diffsModule();
diffs.state = { ...diffs.state, ...state };
diffs.actions = { ...diffs.actions, ...actions };
......@@ -31,18 +38,38 @@ describe('EE DiffRow', () => {
getters,
});
wrapper = shallowMount(DiffRow, { propsData: { ...defaultProps, ...props }, store, provide });
window.gon = { features: { codequalityMrDiffAnnotations } };
wrapper = shallowMount(DiffRow, {
propsData: { ...defaultProps, ...props },
store,
listeners: {
enterdragging: () => {},
stopdragging: () => {},
showCommentForm: () => {},
setHighlightedRow: () => {},
},
});
};
afterEach(() => {
wrapper.destroy();
wrapper = null;
window.gon = {};
Object.values(DiffRow).forEach(({ cache }) => {
if (cache) {
cache.clear();
}
});
});
describe('with feature flag enabled', () => {
beforeEach(() => {
createComponent({
props: { line: { right: { codequality: [{ severity: 'critical' }] } } },
provide: { glFeatures: { codequalityMrDiffAnnotations: true } },
codequalityMrDiffAnnotations: true,
});
});
......@@ -55,7 +82,7 @@ describe('EE DiffRow', () => {
beforeEach(() => {
createComponent({
props: { line: { right: { codequality: [{ severity: 'critical' }] } } },
provide: { glFeatures: { codequalityMrDiffAnnotations: false } },
codequalityMrDiffAnnotations: false,
});
});
......
......@@ -8,6 +8,12 @@ import diffsModule from '~/diffs/store/modules';
import { findInteropAttributes } from '../find_interop_attributes';
import diffFileMockData from '../mock_data/diff_file';
const showCommentForm = jest.fn();
const enterdragging = jest.fn();
const stopdragging = jest.fn();
const setHighlightedRow = jest.fn();
let wrapper;
describe('DiffRow', () => {
const testLines = [
{
......@@ -29,7 +35,7 @@ describe('DiffRow', () => {
},
];
const createWrapper = ({ props, state, actions, isLoggedIn = true }) => {
const createWrapper = ({ props, state = {}, actions, isLoggedIn = true }) => {
Vue.use(Vuex);
const diffs = diffsModule();
......@@ -43,11 +49,25 @@ describe('DiffRow', () => {
getters,
});
window.gon = { current_user_id: isLoggedIn ? 1 : 0 };
const coverageFileData = state.coverageFiles?.files ? state.coverageFiles.files : {};
const propsData = {
fileHash: 'abc',
filePath: 'abc',
line: {},
index: 0,
isHighlighted: false,
fileLineCoverage: (file, line) => {
const hits = coverageFileData[file]?.[line];
if (hits) {
return { text: `Test coverage: ${hits} hits`, class: 'coverage' };
} else if (hits === 0) {
return { text: 'No test coverage', class: 'no-coverage' };
}
return {};
},
...props,
};
......@@ -55,49 +75,37 @@ describe('DiffRow', () => {
glFeatures: { dragCommentSelection: true },
};
return shallowMount(DiffRow, { propsData, store, provide });
return shallowMount(DiffRow, {
propsData,
store,
provide,
listeners: {
enterdragging,
stopdragging,
setHighlightedRow,
showCommentForm,
},
});
};
it('isHighlighted returns true given line.left', () => {
const props = {
line: {
left: {
line_code: 'abc',
},
},
};
const state = { highlightedRow: 'abc' };
const wrapper = createWrapper({ props, state });
expect(wrapper.vm.isHighlighted).toBe(true);
});
afterEach(() => {
wrapper.destroy();
wrapper = null;
it('isHighlighted returns true given line.right', () => {
const props = {
line: {
right: {
line_code: 'abc',
},
},
};
const state = { highlightedRow: 'abc' };
const wrapper = createWrapper({ props, state });
expect(wrapper.vm.isHighlighted).toBe(true);
});
window.gon = {};
showCommentForm.mockReset();
enterdragging.mockReset();
stopdragging.mockReset();
setHighlightedRow.mockReset();
it('isHighlighted returns false given line.left', () => {
const props = {
line: {
left: {
line_code: 'abc',
},
},
};
const wrapper = createWrapper({ props });
expect(wrapper.vm.isHighlighted).toBe(false);
Object.values(DiffRow).forEach(({ cache }) => {
if (cache) {
cache.clear();
}
});
});
const getCommentButton = (wrapper, side) =>
wrapper.find(`[data-testid="${side}-comment-button"]`);
const getCommentButton = (side) => wrapper.find(`[data-testid="${side}-comment-button"]`);
describe.each`
side
......@@ -105,33 +113,30 @@ describe('DiffRow', () => {
${'right'}
`('$side side', ({ side }) => {
it(`renders empty cells if ${side} is unavailable`, () => {
const wrapper = createWrapper({ props: { line: testLines[2], inline: false } });
wrapper = createWrapper({ props: { line: testLines[2], inline: false } });
expect(wrapper.find(`[data-testid="${side}-line-number"]`).exists()).toBe(false);
expect(wrapper.find(`[data-testid="${side}-empty-cell"]`).exists()).toBe(true);
});
describe('comment button', () => {
const showCommentForm = jest.fn();
let line;
beforeEach(() => {
showCommentForm.mockReset();
// https://eslint.org/docs/rules/prefer-destructuring#when-not-to-use-it
// eslint-disable-next-line prefer-destructuring
line = testLines[3];
});
it('renders', () => {
const wrapper = createWrapper({ props: { line, inline: false } });
expect(getCommentButton(wrapper, side).exists()).toBe(true);
wrapper = createWrapper({ props: { line, inline: false } });
expect(getCommentButton(side).exists()).toBe(true);
});
it('responds to click and keyboard events', async () => {
const wrapper = createWrapper({
wrapper = createWrapper({
props: { line, inline: false },
actions: { showCommentForm },
});
const commentButton = getCommentButton(wrapper, side);
const commentButton = getCommentButton(side);
await commentButton.trigger('click');
await commentButton.trigger('keydown.enter');
......@@ -142,11 +147,10 @@ describe('DiffRow', () => {
it('ignores click and keyboard events when comments are disabled', async () => {
line[side].commentsDisabled = true;
const wrapper = createWrapper({
wrapper = createWrapper({
props: { line, inline: false },
actions: { showCommentForm },
});
const commentButton = getCommentButton(wrapper, side);
const commentButton = getCommentButton(side);
await commentButton.trigger('click');
await commentButton.trigger('keydown.enter');
......@@ -157,19 +161,20 @@ describe('DiffRow', () => {
});
it('renders avatars', () => {
const wrapper = createWrapper({ props: { line: testLines[0], inline: false } });
wrapper = createWrapper({ props: { line: testLines[0], inline: false } });
expect(wrapper.find(`[data-testid="${side}-discussions"]`).exists()).toBe(true);
});
});
it('renders left line numbers', () => {
const wrapper = createWrapper({ props: { line: testLines[0] } });
wrapper = createWrapper({ props: { line: testLines[0] } });
const lineNumber = testLines[0].left.old_line;
expect(wrapper.find(`[data-linenumber="${lineNumber}"]`).exists()).toBe(true);
});
it('renders right line numbers', () => {
const wrapper = createWrapper({ props: { line: testLines[0] } });
wrapper = createWrapper({ props: { line: testLines[0] } });
const lineNumber = testLines[0].right.new_line;
expect(wrapper.find(`[data-linenumber="${lineNumber}"]`).exists()).toBe(true);
});
......@@ -186,12 +191,10 @@ describe('DiffRow', () => {
${'left'}
${'right'}
`('emits `enterdragging` onDragEnter $side side', ({ side }) => {
const expectation = { ...line[side], index: 0 };
const wrapper = createWrapper({ props: { line } });
wrapper = createWrapper({ props: { line } });
fireEvent.dragEnter(getByTestId(wrapper.element, `${side}-side`));
expect(wrapper.emitted().enterdragging).toBeTruthy();
expect(wrapper.emitted().enterdragging[0]).toEqual([expectation]);
expect(enterdragging).toHaveBeenCalledWith({ ...line[side], index: 0 });
});
it.each`
......@@ -199,10 +202,10 @@ describe('DiffRow', () => {
${'left'}
${'right'}
`('emits `stopdragging` onDrop $side side', ({ side }) => {
const wrapper = createWrapper({ props: { line } });
wrapper = createWrapper({ props: { line } });
fireEvent.dragEnd(getByTestId(wrapper.element, `${side}-side`));
expect(wrapper.emitted().stopdragging).toBeTruthy();
expect(stopdragging).toHaveBeenCalled();
});
});
......@@ -231,7 +234,7 @@ describe('DiffRow', () => {
it('for lines with coverage', () => {
const coverageFiles = { files: { [name]: { [line]: 5 } } };
const wrapper = createWrapper({ props, state: { coverageFiles } });
wrapper = createWrapper({ props, state: { coverageFiles } });
const coverage = wrapper.find('.line-coverage.right-side');
expect(coverage.attributes('title')).toContain('Test coverage: 5 hits');
......@@ -240,7 +243,7 @@ describe('DiffRow', () => {
it('for lines without coverage', () => {
const coverageFiles = { files: { [name]: { [line]: 0 } } };
const wrapper = createWrapper({ props, state: { coverageFiles } });
wrapper = createWrapper({ props, state: { coverageFiles } });
const coverage = wrapper.find('.line-coverage.right-side');
expect(coverage.attributes('title')).toContain('No test coverage');
......@@ -249,7 +252,7 @@ describe('DiffRow', () => {
it('for unknown lines', () => {
const coverageFiles = {};
const wrapper = createWrapper({ props, state: { coverageFiles } });
wrapper = createWrapper({ props, state: { coverageFiles } });
const coverage = wrapper.find('.line-coverage.right-side');
expect(coverage.attributes('title')).toBeFalsy();
......@@ -267,7 +270,7 @@ describe('DiffRow', () => {
${'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 } });
wrapper = createWrapper({ props: { line, inline } });
expect(findInteropAttributes(wrapper, '[data-testid="left-side"]')).toEqual(leftSide);
expect(findInteropAttributes(wrapper, '[data-testid="right-side"]')).toEqual(rightSide);
......
......@@ -11,24 +11,21 @@ const LINE_CODE = 'abc123';
describe('isHighlighted', () => {
it('should return true if line is highlighted', () => {
const state = { diffs: { highlightedRow: LINE_CODE } };
const line = { line_code: LINE_CODE };
const isCommented = false;
expect(utils.isHighlighted(state, line, isCommented)).toBe(true);
expect(utils.isHighlighted(LINE_CODE, line, isCommented)).toBe(true);
});
it('should return false if line is not highlighted', () => {
const state = { diffs: { highlightedRow: 'xxx' } };
const line = { line_code: LINE_CODE };
const isCommented = false;
expect(utils.isHighlighted(state, line, isCommented)).toBe(false);
expect(utils.isHighlighted('xxx', line, isCommented)).toBe(false);
});
it('should return true if isCommented is true', () => {
const state = { diffs: { highlightedRow: 'xxx' } };
const line = { line_code: LINE_CODE };
const isCommented = true;
expect(utils.isHighlighted(state, line, isCommented)).toBe(true);
expect(utils.isHighlighted('xxx', line, isCommented)).toBe(true);
});
});
......@@ -143,19 +140,14 @@ describe('addCommentTooltip', () => {
'Commenting on symbolic links that replace or are replaced by files is currently not supported.';
const brokenRealTooltip =
'Commenting on files that replace or are replaced by symbolic links is currently not supported.';
const commentTooltip = 'Add a comment to this line';
const dragTooltip = 'Add a comment to this line or drag for multiple lines';
it('should return default tooltip', () => {
expect(utils.addCommentTooltip()).toBeUndefined();
});
it('should return comment tooltip', () => {
expect(utils.addCommentTooltip({})).toEqual(commentTooltip);
});
it('should return drag comment tooltip when dragging is enabled', () => {
expect(utils.addCommentTooltip({}, true)).toEqual(dragTooltip);
expect(utils.addCommentTooltip({})).toEqual(dragTooltip);
});
it('should return broken symlink tooltip', () => {
......
......@@ -28,7 +28,7 @@ describe('DiffView', () => {
};
const diffs = {
actions: { showCommentForm },
getters: { commitId: () => 'abc123' },
getters: { commitId: () => 'abc123', fileLineCoverage: () => ({}) },
namespaced: true,
};
const notes = {
......@@ -84,7 +84,7 @@ describe('DiffView', () => {
it('sets `dragStart` onStartDragging', () => {
const wrapper = createWrapper({ diffLines: [{}] });
wrapper.findComponent(DiffRow).vm.$emit('startdragging', { test: true });
wrapper.findComponent(DiffRow).vm.$emit('startdragging', { line: { test: true } });
expect(wrapper.vm.dragStart).toEqual({ test: true });
});
......@@ -92,7 +92,7 @@ describe('DiffView', () => {
const wrapper = createWrapper({ diffLines: [{}] });
const diffRow = getDiffRow(wrapper);
diffRow.$emit('startdragging', { chunk: 0 });
diffRow.$emit('startdragging', { line: { chunk: 0 } });
diffRow.$emit('enterdragging', { chunk: 1 });
expect(setSelectedCommentPosition).not.toHaveBeenCalled();
......@@ -109,7 +109,7 @@ describe('DiffView', () => {
const wrapper = createWrapper({ diffLines: [{}] });
const diffRow = getDiffRow(wrapper);
diffRow.$emit('startdragging', { chunk: 1, index: start });
diffRow.$emit('startdragging', { line: { chunk: 1, index: start } });
diffRow.$emit('enterdragging', { chunk: 1, index: end });
const arg = setSelectedCommentPosition.mock.calls[0][1];
......@@ -122,7 +122,7 @@ describe('DiffView', () => {
const wrapper = createWrapper({ diffLines: [{}] });
const diffRow = getDiffRow(wrapper);
diffRow.$emit('startdragging', { test: true });
diffRow.$emit('startdragging', { line: { test: true } });
expect(wrapper.vm.dragStart).toEqual({ test: true });
diffRow.$emit('stopdragging');
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment