From d0c7631720e88832e5f7b0eca7c963f50ec7a173 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Ctan-nhu=E2=80=9D?= <“tan@harness.io”> Date: Thu, 6 Apr 2023 12:33:20 -0700 Subject: [PATCH 1/2] Performance improvement for GitBlame UI --- .../FileContent/GitBlame.tsx | 132 ++++++++++-------- 1 file changed, 74 insertions(+), 58 deletions(-) diff --git a/web/src/pages/Repository/RepositoryContent/FileContent/GitBlame.tsx b/web/src/pages/Repository/RepositoryContent/FileContent/GitBlame.tsx index c5720ef9a..55330780f 100644 --- a/web/src/pages/Repository/RepositoryContent/FileContent/GitBlame.tsx +++ b/web/src/pages/Repository/RepositoryContent/FileContent/GitBlame.tsx @@ -1,5 +1,4 @@ import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react' -import { throttle } from 'lodash-es' import { Avatar, Container, FontVariation, Layout, StringSubstitute, Text } from '@harness/uicore' import { LanguageDescription } from '@codemirror/language' import { indentWithTab } from '@codemirror/commands' @@ -32,7 +31,7 @@ interface BlameBlock { type BlameBlockRecord = Record -const BLAME_BLOCK_NOT_YET_CALCULATED_TOP_POSITION = -1 +const INITIAL_TOP_POSITION = -1 export const GitBlame: React.FC> = ({ repoMetadata, @@ -55,7 +54,7 @@ export const GitBlame: React.FC { let startLine = lineNumber @@ -81,7 +81,7 @@ export const GitBlame: React.FC { + ({ view, geometryChanged }: ViewUpdate) => { if (geometryChanged) { view.viewportLineBlocks.forEach(lineBlock => { const { from, top, height } = lineBlock @@ -90,9 +90,9 @@ export const GitBlame: React.FC{getErrorMessage(error)} } + // NOTE 1: Using React in combined with CodeMirror is not ideal due to the fact that + // React will re-render the entire component tree on every CodeMirror update. + // We might want to consider using minimal React usage in the future, plus CodeMirror APIs more. + // NOTE 2: Try to improve by using ref instead of state. + return ( {Object.values(blameBlocks) - .filter(({ topPosition }) => topPosition !== BLAME_BLOCK_NOT_YET_CALCULATED_TOP_POSITION) - .map(({ fromLineNumber, topPosition: top, heights, commitInfo }) => { - const height = Object.values(heights).reduce((a, b) => a + b, 0) - - return ( - - - - - - - - - {commitInfo?.title} - - - {commitInfo?.author?.identity?.name as string}, - timestamp: - }} - /> - - - - - - ) - })} + .filter(({ topPosition }) => topPosition !== INITIAL_TOP_POSITION) + .map(blameInfo => ( + + ))} () +const GitBlameSourceViewer = React.memo(function GitBlameSourceViewer({ + source, + filename, + onViewUpdate, + blameBlocks +}: GitBlameSourceViewerProps) { + const view = useRef() const ref = useRef() const languageConfig = useMemo(() => new Compartment(), []) - const lineWidgetSpec = useMemo(() => { - const spec: EditorLinePaddingWidgetSpec[] = [] + + useEffect(() => { + const lineWidgetSpec: EditorLinePaddingWidgetSpec[] = [] Object.values(blameBlocks).forEach(block => { const blockLines = block.numberOfLines - spec.push({ + lineWidgetSpec.push({ lineNumber: block.fromLineNumber, position: LineWidgetPosition.TOP, blockLines }) - spec.push({ + lineWidgetSpec.push({ lineNumber: block.toLineNumber, position: LineWidgetPosition.BOTTOM, blockLines }) }) - return spec - }, [blameBlocks]) - - useEffect(() => { const customLineNumberGutter = gutter({ lineMarker(_view, line) { const lineNumber: number = _view.state.doc.lineAt(line.from).number @@ -282,25 +258,25 @@ function GitBlameSourceViewer({ source, filename, onViewUpdate, blameBlocks }: G parent: ref.current }) - setView(editorView) + view.current = editorView return () => { editorView.destroy() } - }, []) // eslint-disable-line + }, []) // eslint-disable-line react-hooks/exhaustive-deps useEffect(() => { - if (view && filename) { + if (filename) { languageDescriptionFrom(filename) ?.load() .then(languageSupport => { - view.dispatch({ effects: languageConfig.reconfigure(languageSupport) }) + view.current?.dispatch({ effects: languageConfig.reconfigure(languageSupport) }) }) } }, [filename, view, languageConfig]) return -} +}) function languageDescriptionFrom(filename: string) { return LanguageDescription.matchFilename(languages, filename) @@ -339,3 +315,43 @@ class EditorLinePaddingWidget extends WidgetType { return false } } + +function GitBlameMetaInfo({ fromLineNumber, topPosition, heights, commitInfo }: BlameBlock) { + const height = Object.values(heights).reduce((a, b) => a + b, 0) + const { getString } = useStrings() + + return ( + + + + + + + + + {commitInfo?.title} + + + {commitInfo?.author?.identity?.name as string}, + timestamp: + }} + /> + + + + + + ) +} From 1fd425d51ef25abf55bf1745af0016442b85c701 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Ctan-nhu=E2=80=9D?= <“tan@harness.io”> Date: Thu, 6 Apr 2023 14:31:58 -0700 Subject: [PATCH 2/2] Performance improvement for GitBlame UI --- .../FileContent/GitBlame.module.scss | 4 ++ .../FileContent/GitBlame.tsx | 67 +++++++++++-------- 2 files changed, 44 insertions(+), 27 deletions(-) diff --git a/web/src/pages/Repository/RepositoryContent/FileContent/GitBlame.module.scss b/web/src/pages/Repository/RepositoryContent/FileContent/GitBlame.module.scss index bab5043e8..a646a8017 100644 --- a/web/src/pages/Repository/RepositoryContent/FileContent/GitBlame.module.scss +++ b/web/src/pages/Repository/RepositoryContent/FileContent/GitBlame.module.scss @@ -51,6 +51,10 @@ position: absolute; width: 100%; + &[data-block-top='-1'] { + display: none; + } + &::before { position: absolute; z-index: 2; diff --git a/web/src/pages/Repository/RepositoryContent/FileContent/GitBlame.tsx b/web/src/pages/Repository/RepositoryContent/FileContent/GitBlame.tsx index 55330780f..bd47bc123 100644 --- a/web/src/pages/Repository/RepositoryContent/FileContent/GitBlame.tsx +++ b/web/src/pages/Repository/RepositoryContent/FileContent/GitBlame.tsx @@ -54,8 +54,8 @@ export const GitBlame: React.FC { const { from, top, height } = lineBlock const lineNumber = view.state.doc.lineAt(from).number - const blameBlockAtLineNumber = findBlockForLineNumber(lineNumber) + const blockAtLineNumber = findBlockForLineNumber(lineNumber) - if (!blameBlockAtLineNumber) { + if (!blockAtLineNumber) { // eslint-disable-next-line no-console console.error('Bad math! Cannot find a blame block for line', lineNumber) } else { - if (blameBlockAtLineNumber.topPosition === INITIAL_TOP_POSITION) { - blameBlockAtLineNumber.topPosition = top + if (blockAtLineNumber.topPosition === INITIAL_TOP_POSITION) { + blockAtLineNumber.topPosition = top } // CodeMirror reports top position of a block incorrectly sometimes, so we need to normalize it - // using the previous block. + // using dimensions of the previous block. if (lineNumber > 1) { const previousBlock = findBlockForLineNumber(lineNumber - 1) - if (previousBlock.fromLineNumber !== blameBlockAtLineNumber.fromLineNumber) { - const normalizedTop = - previousBlock.topPosition + Object.values(previousBlock.heights).reduce((a, b) => a + b, 0) - - blameBlockAtLineNumber.topPosition = normalizedTop + if (previousBlock.fromLineNumber !== blockAtLineNumber.fromLineNumber) { + blockAtLineNumber.topPosition = previousBlock.topPosition + computeHeight(previousBlock.heights) } } - blameBlockAtLineNumber.heights[lineNumber] = height + blockAtLineNumber.heights[lineNumber] = height + + const blockDOM = document.querySelector( + `.${css.blameBox}[data-block-from-line="${blockAtLineNumber.fromLineNumber}"]` + ) as HTMLDivElement + + if (blockDOM) { + const _height = `${computeHeight(blockAtLineNumber.heights)}px` + const _top = `${blockAtLineNumber.topPosition}px` + + if (blockDOM.style.height !== _height || blockDOM.style.top !== _top) { + blockDOM.style.height = _height + blockDOM.style.top = _top + + if (blockAtLineNumber.topPosition !== INITIAL_TOP_POSITION) { + blockDOM.removeAttribute('data-block-top') + } + } + } } }) - - setBlameBlocks({ ...blameBlocks }) } }, [] // eslint-disable-line react-hooks/exhaustive-deps @@ -129,20 +142,13 @@ export const GitBlame: React.FC{getErrorMessage(error)} } - // NOTE 1: Using React in combined with CodeMirror is not ideal due to the fact that - // React will re-render the entire component tree on every CodeMirror update. - // We might want to consider using minimal React usage in the future, plus CodeMirror APIs more. - // NOTE 2: Try to improve by using ref instead of state. - return ( - {Object.values(blameBlocks) - .filter(({ topPosition }) => topPosition !== INITIAL_TOP_POSITION) - .map(blameInfo => ( - - ))} + {Object.values(blameBlocks).map(blameInfo => ( + + ))} a + b, 0) +function computeHeight(heights: Record) { + return Object.values(heights).reduce((a, b) => a + b, 0) +} + +function GitBlameMetaInfo({ fromLineNumber, toLineNumber, topPosition, heights, commitInfo }: BlameBlock) { + const height = computeHeight(heights) const { getString } = useStrings() return (