From f626dc640e59f4cc3bc393284c4e2519e1b3078c Mon Sep 17 00:00:00 2001 From: Tan Nhu Date: Fri, 9 Aug 2024 17:41:59 +0000 Subject: [PATCH] Render off-screen diffs in a single pre tag to boost performance (#2271) * Tan's last PR :) * Sync with main * Prevent browser crash on big PR (>200 files changed) * Fixed link to comment not working * Add NPE prevention * Fixed Show Diff works only once * Fix Harness nav is not auto-collapsed when visiting PR Changes * Add code to restore new and edited comments * Render off-screen diffs in a single pre tag to boost performance --- web/src/Config.ts | 2 +- web/src/components/Changes/Changes.tsx | 14 +- web/src/components/CommentBox/CommentBox.tsx | 183 +++++++++++++++--- .../CommentBox/comment-resolved.svg | 2 +- .../DiffViewer/DiffViewer.module.scss | 14 +- .../DiffViewer/DiffViewer.module.scss.d.ts | 2 +- web/src/components/DiffViewer/DiffViewer.tsx | 176 ++++++++++++----- .../DiffViewer/InViewDiffBlockRenderer.tsx | 7 +- .../DiffViewer/usePullReqComments.tsx | 46 ++++- web/src/hooks/useIsSidebarExpanded.ts | 4 +- 10 files changed, 355 insertions(+), 95 deletions(-) diff --git a/web/src/Config.ts b/web/src/Config.ts index 72fc23a53..72a20fd46 100644 --- a/web/src/Config.ts +++ b/web/src/Config.ts @@ -31,7 +31,7 @@ export default { PULL_REQUEST_DIFF_RENDERING_BLOCK_SIZE: 10, /** Detection margin for on-screen / off-screen rendering optimization. In pixels. */ - IN_VIEWPORT_DETECTION_MARGIN: 5_000, + IN_VIEWPORT_DETECTION_MARGIN: 256_000, /** Limit for the secret input in bytes */ SECRET_LIMIT_IN_BYTES: 5_242_880 diff --git a/web/src/components/Changes/Changes.tsx b/web/src/components/Changes/Changes.tsx index 4494217ba..c1fbe2d6e 100644 --- a/web/src/components/Changes/Changes.tsx +++ b/web/src/components/Changes/Changes.tsx @@ -390,7 +390,7 @@ const ChangesInternal: React.FC = ({ } scheduleTask(() => { - if (isMounted.current && loopCount++ < 50) { + if (isMounted.current && loopCount++ < 100) { if ( !outterBlockDOM || !innerBlockDOM || @@ -548,14 +548,16 @@ const ChangesInternal: React.FC = ({ key={key} blockName={outterBlockName(blockIndex)} root={scrollElementRef as RefObject} - shouldRetainChildren={shouldRetainDiffChildren}> + shouldRetainChildren={shouldRetainDiffChildren} + detectionMargin={calculateDetectionMargin(diffs?.length as number)}> {diffsBlock.map((diff, index) => { return ( + shouldRetainChildren={shouldRetainDiffChildren} + detectionMargin={Config.IN_VIEWPORT_DETECTION_MARGIN}> 0} // render in readonly mode in case a commit is selected diff={diff} @@ -618,6 +620,12 @@ const outterBlockName = (blockIndex: number) => `outter-${blockIndex}` const innerBlockName = (filePath: string) => `inner-${filePath}` const { scheduleTask } = createRequestIdleCallbackTaskPool() +// If there are more than 200 diffs, we decrease the detection margin to make sure browser do not crash. As a result, Cmd-F +// won't work well on diffs that got hidden/out of viewport. +// TODO: This could be more accurate to calculate based on the complexity of the diff contents (added/deleted lines)? +const calculateDetectionMargin = (diffsLength: number) => + diffsLength >= 200 ? 5000 : Config.IN_VIEWPORT_DETECTION_MARGIN + // Workaround util to correct filePath which is not correctly produced by // git itself when filename contains space // @see https://stackoverflow.com/questions/77596606/why-does-git-add-trailing-tab-to-the-b-line-of-the-diff-when-the-file-nam diff --git a/web/src/components/CommentBox/CommentBox.tsx b/web/src/components/CommentBox/CommentBox.tsx index 40a46186d..e4aca3e88 100644 --- a/web/src/components/CommentBox/CommentBox.tsx +++ b/web/src/components/CommentBox/CommentBox.tsx @@ -14,7 +14,7 @@ * limitations under the License. */ -import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react' +import React, { createRef, useCallback, useEffect, useMemo, useRef, useState } from 'react' import type { EditorView } from '@codemirror/view' import { Render, Match, Truthy, Falsy, Else } from 'react-jsx-match' import { Container, Layout, Avatar, TextInput, Text, FlexExpander, Button, useIsMounted } from '@harnessio/uicore' @@ -34,6 +34,7 @@ import { ButtonRoleProps, CodeCommentState } from 'utils/Utils' import { useResizeObserver } from 'hooks/useResizeObserver' import { useCustomEventListener } from 'hooks/useEventListener' import type { SuggestionBlock } from 'components/SuggestionBlock/SuggestionBlock' +import type { CommentRestorationTrackingState, DiffViewerExchangeState } from 'components/DiffViewer/DiffViewer' import commentActiveIconUrl from './comment.svg?url' import commentResolvedIconUrl from './comment-resolved.svg?url' import css from './CommentBox.module.scss' @@ -83,6 +84,7 @@ interface CommentBoxProps { resetOnSave?: boolean hideCancel?: boolean currentUserName: string + commentThreadId?: number commentItems: CommentItem[] handleAction: ( action: CommentAction, @@ -99,6 +101,8 @@ interface CommentBoxProps { routingId: string copyLinkToComment: (commentId: number, commentItem: CommentItem) => void suggestionBlock?: SuggestionBlock + memorizedState?: CommentRestorationTrackingState + commentsVisibilityAtLineNumber?: DiffViewerExchangeState['commentsVisibilityAtLineNumber'] } const CommentBoxInternal = ({ @@ -109,6 +113,7 @@ const CommentBoxInternal = ({ initialContent = '', width, fluid, + commentThreadId, commentItems = [], currentUserName, handleAction, @@ -123,7 +128,9 @@ const CommentBoxInternal = ({ standalone, routingId, copyLinkToComment, - suggestionBlock + suggestionBlock, + memorizedState, + commentsVisibilityAtLineNumber }: CommentBoxProps) => { const { getString } = useStrings() const [comments, setComments] = useState[]>(commentItems) @@ -134,6 +141,13 @@ const CommentBoxInternal = ({ const containerRef = useRef(null) const isMounted = useIsMounted() + const clearMemorizedState = useCallback(() => { + if (memorizedState) { + delete memorizedState.showReplyPlaceHolder + delete memorizedState.uncommittedText + } + }, [memorizedState]) + useResizeObserver( containerRef, useCallback( @@ -156,10 +170,13 @@ const CommentBoxInternal = ({ const _onCancel = useCallback(() => { setMarkdown('') setShowReplyPlaceHolder(true) + + clearMemorizedState() + if (onCancel && !comments.length) { onCancel() } - }, [setShowReplyPlaceHolder, onCancel, comments.length]) + }, [setShowReplyPlaceHolder, onCancel, comments.length, clearMemorizedState]) const hidePlaceHolder = useCallback(() => setShowReplyPlaceHolder(false), [setShowReplyPlaceHolder]) const onQuote = useCallback((content: string) => { const replyContent = content @@ -179,13 +196,73 @@ const CommentBoxInternal = ({ }) }, [dirties, setDirty]) + useEffect( + // This function restores CommentBox internal states from memorizedState + // after it got destroyed during HTML/textContent serialization/deserialization + // This approach is not optimized, we probably have to think about a shared + // store per diff or something else to make the flow nicer + function serializeNewCommentInfo() { + if (!commentThreadId || !memorizedState) return + + if (commentThreadId < 0) { + if (!comments?.[0]?.id) { + if (!markdown && memorizedState.uncommittedText) { + setMarkdown(memorizedState.uncommittedText) + viewRef.current?.dispatch({ + changes: { + from: 0, + to: viewRef.current.state.doc.length, + insert: memorizedState.uncommittedText + } + }) + viewRef.current?.contentDOM?.blur() + } else { + memorizedState.uncommittedText = markdown + memorizedState.showReplyPlaceHolder = showReplyPlaceHolder + } + } else { + clearMemorizedState() + } + } else if (commentThreadId > 0) { + if (!showReplyPlaceHolder) { + if (markdown) { + memorizedState.uncommittedText = markdown + memorizedState.showReplyPlaceHolder = false + } + } else { + if (!markdown && memorizedState.showReplyPlaceHolder === false) { + setShowReplyPlaceHolder(false) + + const { uncommittedText = '' } = memorizedState + + setTimeout(() => { + setMarkdown(uncommittedText) + viewRef.current?.dispatch({ + changes: { + from: 0, + to: viewRef.current.state.doc.length, + insert: uncommittedText + } + }) + viewRef.current?.contentDOM?.blur() + }, 0) + } + + delete memorizedState.showReplyPlaceHolder + delete memorizedState.uncommittedText + } + } + }, + [markdown, commentThreadId, comments, memorizedState, clearMemorizedState, showReplyPlaceHolder] + ) + return ( + data-comment-thread-id={comments?.[0]?.id || commentThreadId || ''}> {outlets[CommentBoxOutletPosition.TOP]} @@ -210,6 +287,8 @@ const CommentBoxInternal = ({ outlets={outlets} copyLinkToComment={copyLinkToComment} suggestionBlock={suggestionBlock} + memorizedState={memorizedState} + commentsVisibilityAtLineNumber={commentsVisibilityAtLineNumber} /> @@ -250,6 +329,8 @@ const CommentBoxInternal = ({ value={markdown} onChange={setMarkdown} onSave={async (value: string) => { + clearMemorizedState() + if (handleAction) { const [result, updatedItem] = await handleAction( comments.length ? CommentAction.REPLY : CommentAction.NEW, @@ -306,7 +387,13 @@ const CommentBoxInternal = ({ interface CommentsThreadProps extends Pick< CommentBoxProps, - 'commentItems' | 'handleAction' | 'outlets' | 'copyLinkToComment' | 'suggestionBlock' + | 'commentItems' + | 'handleAction' + | 'outlets' + | 'copyLinkToComment' + | 'suggestionBlock' + | 'memorizedState' + | 'commentsVisibilityAtLineNumber' > { onQuote: (content: string) => void setDirty: (index: number, dirty: boolean) => void @@ -321,21 +408,26 @@ const CommentsThread = ({ outlets = {}, repoMetadata, copyLinkToComment, - suggestionBlock + suggestionBlock, + memorizedState, + commentsVisibilityAtLineNumber }: CommentsThreadProps) => { const { getString } = useStrings() const { standalone, routingId } = useAppContext() const [editIndexes, setEditIndexes] = useState>({}) const resetStateAtIndex = useCallback( - (index: number) => { + (index: number, commentItem: CommentItem) => { delete editIndexes[index] setEditIndexes({ ...editIndexes }) + + if (memorizedState?.uncommittedEditComments && commentItem?.id) { + memorizedState.uncommittedEditComments.delete(commentItem.id) + } }, - [editIndexes] + [editIndexes, memorizedState] ) const isCommentThreadResolved = useMemo(() => !!get(commentItems[0], 'payload.resolved'), [commentItems]) const domRef = useRef() - const show = useRef(isCommentThreadResolved ? false : true) const internalFlags = useRef({ initialized: false }) useEffect( @@ -353,18 +445,18 @@ const CommentsThread = ({ const lineNumColDOM = annotatedRow.firstElementChild as HTMLElement const sourceLineNumber = annotatedRow.dataset.sourceLineNumber const button: HTMLButtonElement = lineNumColDOM?.querySelector('button') || document.createElement('button') + const showFromMemory = commentsVisibilityAtLineNumber?.get(Number(sourceLineNumber)) + let show = showFromMemory !== undefined ? showFromMemory : isCommentThreadResolved ? false : true if (!button.onclick) { const toggleHidden = (dom: Element) => { - if (show.current) dom.setAttribute('hidden', '') + if (show) dom.setAttribute('hidden', '') else dom.removeAttribute('hidden') } const toggleComments = (e: KeyboardEvent | MouseEvent) => { let commentRow = annotatedRow.nextElementSibling as HTMLElement while (commentRow?.dataset?.annotatedLine) { - toggleHidden(commentRow) - // Toggle opposite place-holder as well const diffParent = commentRow.closest('.d2h-code-wrapper')?.parentElement const oppositeDiv = diffParent?.classList.contains('right') @@ -376,11 +468,16 @@ const CommentsThread = ({ oppositePlaceHolders?.forEach(dom => toggleHidden(dom)) + toggleHidden(commentRow) commentRow = commentRow.nextElementSibling as HTMLElement } - show.current = !show.current + show = !show - if (!show.current) button.dataset.threadsCount = String(activeThreads + resolvedThreads) + if (memorizedState) { + commentsVisibilityAtLineNumber?.set(Number(sourceLineNumber), show) + } + + if (!show) button.dataset.threadsCount = String(activeThreads + resolvedThreads) else delete button.dataset.threadsCount e.stopPropagation() @@ -388,6 +485,7 @@ const CommentsThread = ({ button.classList.add(css.toggleComment) button.title = getString('pr.toggleComments') + button.dataset.toggleComment = 'true' button.addEventListener('keydown', e => { if (e.key === 'Enter') toggleComments(e) @@ -404,7 +502,9 @@ const CommentsThread = ({ while (commentRow?.dataset?.annotatedLine) { if (commentRow.dataset.commentThreadStatus == CodeCommentState.RESOLVED) { resolvedThreads++ - if (!internalFlags.current.initialized) show.current = false + if (!internalFlags.current.initialized && !showFromMemory) { + show = false + } } else activeThreads++ commentRow = commentRow.nextElementSibling as HTMLElement @@ -415,19 +515,44 @@ const CommentsThread = ({ if (!internalFlags.current.initialized) { internalFlags.current.initialized = true - if (!show.current && resolvedThreads) button.dataset.threadsCount = String(resolvedThreads) + if (!show && resolvedThreads) button.dataset.threadsCount = String(resolvedThreads) else delete button.dataset.threadsCount } } }, - [isCommentThreadResolved, getString] + [isCommentThreadResolved, getString, commentsVisibilityAtLineNumber, memorizedState] ) + const viewRefs = useRef( + Object.fromEntries( + commentItems.map(commentItem => [commentItem.id, createRef() as React.MutableRefObject]) + ) + ) + const contentRestoredRefs = useRef>({}) return ( {commentItems.map((commentItem, index) => { const isLastItem = index === commentItems.length - 1 + const contentFromMemorizedState = memorizedState?.uncommittedEditComments?.get(commentItem.id) + const viewRef = viewRefs.current[commentItem.id] + + if (viewRef && contentFromMemorizedState !== undefined && !contentRestoredRefs.current[commentItem.id]) { + editIndexes[index] = true + contentRestoredRefs.current[commentItem.id] = true + + setTimeout(() => { + if (contentFromMemorizedState !== commentItem.content) { + viewRef.current?.dispatch({ + changes: { + from: 0, + to: viewRef.current.state.doc.length, + insert: contentFromMemorizedState + } + }) + } + }, 0) + } return ( ({ className: cx(css.optionMenuIcon, css.edit), iconName: 'Edit', text: getString('edit'), - onClick: () => setEditIndexes({ ...editIndexes, ...{ [index]: true } }) + onClick: () => { + setEditIndexes({ ...editIndexes, ...{ [index]: true } }) + if (memorizedState) { + memorizedState.uncommittedEditComments = + memorizedState.uncommittedEditComments || new Map() + memorizedState.uncommittedEditComments.set(commentItem.id, commentItem.content) + } + } }, { hasIcon: true, @@ -512,7 +644,7 @@ const CommentsThread = ({ text: getString('delete'), onClick: async () => { if (await handleAction(CommentAction.DELETE, '', commentItem)) { - resetStateAtIndex(index) + resetStateAtIndex(index, commentItem) } } } @@ -538,13 +670,20 @@ const CommentsThread = ({ standalone={standalone} repoMetadata={repoMetadata} value={commentItem?.content} + viewRef={viewRefs.current[commentItem.id]} onSave={async value => { if (await handleAction(CommentAction.UPDATE, value, commentItem)) { commentItem.content = value - resetStateAtIndex(index) + resetStateAtIndex(index, commentItem) } }} - onCancel={() => resetStateAtIndex(index)} + onChange={value => { + if (memorizedState) { + memorizedState.uncommittedEditComments = memorizedState.uncommittedEditComments || new Map() + memorizedState.uncommittedEditComments.set(commentItem.id, value) + } + }} + onCancel={() => resetStateAtIndex(index, commentItem)} setDirty={_dirty => { setDirty(index, _dirty) }} @@ -555,7 +694,7 @@ const CommentsThread = ({ save: getString('save'), cancel: getString('cancel') }} - autoFocusAndPosition + autoFocusAndPosition={contentFromMemorizedState ? false : true} suggestionBlock={suggestionBlock} /> diff --git a/web/src/components/CommentBox/comment-resolved.svg b/web/src/components/CommentBox/comment-resolved.svg index 909aca598..e2e602b8a 100644 --- a/web/src/components/CommentBox/comment-resolved.svg +++ b/web/src/components/CommentBox/comment-resolved.svg @@ -1 +1 @@ - \ No newline at end of file + \ No newline at end of file diff --git a/web/src/components/DiffViewer/DiffViewer.module.scss b/web/src/components/DiffViewer/DiffViewer.module.scss index 3baa73092..c9f0ceaec 100644 --- a/web/src/components/DiffViewer/DiffViewer.module.scss +++ b/web/src/components/DiffViewer/DiffViewer.module.scss @@ -465,14 +465,12 @@ border-bottom-right-radius: 4px; max-width: calc(var(--page-container-width) - 48px); - &.hidden { - height: var(--block-height); - content-visibility: auto; - contain-intrinsic-size: auto var(--line-height); - - * { - display: none !important; - } + .offscreenText { + font-size: 12px; + white-space: normal; + line-height: 20px; + margin: 0; + color: transparent; } } } diff --git a/web/src/components/DiffViewer/DiffViewer.module.scss.d.ts b/web/src/components/DiffViewer/DiffViewer.module.scss.d.ts index 828fd013c..af3a717b7 100644 --- a/web/src/components/DiffViewer/DiffViewer.module.scss.d.ts +++ b/web/src/components/DiffViewer/DiffViewer.module.scss.d.ts @@ -26,8 +26,8 @@ export declare const expandCollapseDiffBtn: string export declare const fileChanged: string export declare const fname: string export declare const fnamePopover: string -export declare const hidden: string export declare const main: string +export declare const offscreenText: string export declare const popover: string export declare const readOnly: string export declare const selectoSelection: string diff --git a/web/src/components/DiffViewer/DiffViewer.tsx b/web/src/components/DiffViewer/DiffViewer.tsx index 23ff77170..6f9fb43c7 100644 --- a/web/src/components/DiffViewer/DiffViewer.tsx +++ b/web/src/components/DiffViewer/DiffViewer.tsx @@ -41,15 +41,15 @@ import { useStrings } from 'framework/strings' import { CodeIcon, GitInfoProps } from 'utils/GitUtils' import type { DiffFileEntry } from 'utils/types' import { useAppContext } from 'AppContext' -import type { GitFileDiff, TypesPullReq } from 'services/code' +import type { GitFileDiff, TypesPullReq, TypesPullReqActivity } from 'services/code' import { CopyButton } from 'components/CopyButton/CopyButton' import { NavigationCheck } from 'components/NavigationCheck/NavigationCheck' import type { UseGetPullRequestInfoResult } from 'pages/PullRequest/useGetPullRequestInfo' import { useQueryParams } from 'hooks/useQueryParams' -import { useCustomEventListener } from 'hooks/useEventListener' +import { useCustomEventListener, useEventListener } from 'hooks/useEventListener' import { useShowRequestError } from 'hooks/useShowRequestError' import { getErrorMessage, isInViewport } from 'utils/Utils' -import { createRequestIdleCallbackTaskPool } from 'utils/Task' +import { createRequestAnimationFrameTaskPool } from 'utils/Task' import { useResizeObserver } from 'hooks/useResizeObserver' import { useFindGitBranch } from 'hooks/useFindGitBranch' import Config from 'Config' @@ -58,7 +58,8 @@ import { DIFF_VIEWER_HEADER_HEIGHT, ViewStyle, getFileViewedState, - FileViewedState + FileViewedState, + DiffCommentItem } from './DiffViewerUtils' import { usePullReqComments } from './usePullReqComments' import Collapse from '../../icons/collapse.svg' @@ -165,6 +166,7 @@ const DiffViewerInternal: React.FC = ({ }, [ref] ) + const contentHTML = useRef(null) useResizeObserver( contentRef, @@ -178,20 +180,6 @@ const DiffViewerInternal: React.FC = ({ ) ) - useEffect(() => { - let taskId = 0 - if (inView) { - taskId = scheduleLowPriorityTask(() => { - if (isMounted.current && contentRef.current) contentRef.current.classList.remove(css.hidden) - }) - } else { - taskId = scheduleLowPriorityTask(() => { - if (isMounted.current && contentRef.current) contentRef.current.classList.add(css.hidden) - }) - } - return () => cancelTask(taskId) - }, [inView, isMounted]) - // // Handling custom events sent to DiffViewer from external components/features // such as "jump to file", "jump to comment", etc... @@ -202,6 +190,21 @@ const DiffViewerInternal: React.FC = ({ const { action, commentId } = event.detail const containerDOM = document.getElementById(diff.containerId) as HTMLDivElement + function scrollToComment(count = 0) { + const commentDOM = containerDOM.querySelector(`[data-comment-id="${commentId}"]`) as HTMLDivElement + + if (!isMounted.current || count > 100) { + return + } + + if (commentDOM) { + const dom = commentDOM?.parentElement?.parentElement?.parentElement?.parentElement + if (dom) dom.lastElementChild?.scrollIntoView({ block: 'center' }) + } else { + setTimeout(() => scrollToComment(count + 1), 100) + } + } + function scrollToContainer() { if (!isMounted.current) return @@ -215,10 +218,7 @@ const DiffViewerInternal: React.FC = ({ scrollElement.scroll({ top: (scrollElement.scrollTop || window.scrollY) + scrollGap }) } } else { - const commentDOM = containerDOM.querySelector(`[data-comment-id="${commentId}"]`) as HTMLDivElement - // dom is the great grand parent of the comment DOM (CommentBox) - const dom = commentDOM?.parentElement?.parentElement?.parentElement?.parentElement - if (dom) dom.lastElementChild?.scrollIntoView({ block: 'center' }) + scrollToComment() } } @@ -247,7 +247,8 @@ const DiffViewerInternal: React.FC = ({ containerRef, contentRef, refetchActivities, - setDirty + setDirty, + memorizedState }) useEffect( @@ -290,7 +291,7 @@ const DiffViewerInternal: React.FC = ({ if (isInViewport(containerRef.current as Element, 1000)) { renderDiffAndComments() } else { - taskId = scheduleLowPriorityTask(renderDiffAndComments) + taskId = scheduleTask(renderDiffAndComments) } } @@ -316,6 +317,69 @@ const DiffViewerInternal: React.FC = ({ const branchInfo = useFindGitBranch(pullReqMetadata?.source_branch) + useEffect( + function serializeDeserializeContent() { + const dom = contentRef.current + + if (inView) { + if (isMounted.current && dom && contentHTML.current) { + dom.innerHTML = contentHTML.current + contentHTML.current = null + + // Remove all signs from the raw HTML that CommentBox was mounted so + // it can be mounted/re-rendered again freshly + dom.querySelectorAll('tr[data-source-line-number]').forEach(row => { + row.removeAttribute('data-source-line-number') + row.removeAttribute('data-comment-ids') + row.querySelector('button[data-toggle-comment="true"]')?.remove?.() + }) + dom.querySelectorAll('tr[data-annotated-line],tr[data-place-holder-for-line]').forEach(row => { + row.remove?.() + }) + + // Attach comments again + commentsHook.current.attachAllCommentThreads() + } + } else { + if (isMounted.current && dom && !contentHTML.current) { + const { clientHeight, textContent, innerHTML } = dom + + // Detach comments since they are no longer in sync in DOM as + // all DOMs are removed + commentsHook.current.detachAllCommentThreads() + + // Save current innerHTML + contentHTML.current = innerHTML + + const pre = document.createElement('pre') + pre.style.height = clientHeight + 'px' + pre.textContent = textContent + pre.classList.add(css.offscreenText) + + dom.textContent = '' + dom.appendChild(pre) + + // TODO: Might be good to clean textContent a bit to not include + // diff header info, line numbers, hunk headers, etc... + } + } + }, + [inView, isMounted, commentsHook] + ) + + // Add click event listener from contentRef to handle click event on "Show Diff" button + // This can't be done from the button itself because it got serialized / deserialized from + // text during off-screen optimization (handler will be gone/destroyed) + useEventListener( + 'click', + useCallback(function showDiff(event) { + if (((event.target as HTMLElement)?.closest('button') as HTMLElement)?.dataset?.action === ACTION_SHOW_DIFF) { + setRenderCustomContent(false) + } + }, []), + contentRef.current as HTMLDivElement + ) + useShowRequestError(fullDiffError, 0) useEffect( @@ -336,7 +400,10 @@ const DiffViewerInternal: React.FC = ({ if (memorizedState.get(diff.filePath)?.collapsed) { setCollapsed(false) - memorizedState.set(diff.filePath, { ...memorizedState.get(diff.filePath), collapsed: false }) + memorizedState.set(diff.filePath, { + ...memorizedState.get(diff.filePath), + collapsed: false + }) } } catch (exception) { showError(getErrorMessage(exception), 0) @@ -505,28 +572,32 @@ const DiffViewerInternal: React.FC = ({ - - - - - - - - {getString( - fileDeleted - ? 'pr.fileDeleted' - : isDiffTooLarge || diffHasVeryLongLine - ? 'pr.diffTooLarge' - : isBinary - ? 'pr.fileBinary' - : 'pr.fileUnchanged' - )} - - - - + {/* Note: This parent container is needed to make sure "Show Diff" work correctly + with content converted between textContent and innerHTML */} + + + + + + + + + {getString( + fileDeleted + ? 'pr.fileDeleted' + : isDiffTooLarge || diffHasVeryLongLine + ? 'pr.diffTooLarge' + : isBinary + ? 'pr.fileBinary' + : 'pr.fileUnchanged' + )} + + + + + @@ -535,6 +606,7 @@ const DiffViewerInternal: React.FC = ({ } const BLOCK_HEIGHT = '--block-height' +const ACTION_SHOW_DIFF = 'showDiff' export enum DiffViewerEvent { SCROLL_INTO_VIEW = 'scrollIntoView' @@ -549,8 +621,16 @@ export interface DiffViewerExchangeState { collapsed?: boolean useFullDiff?: boolean fullDiff?: DiffFileEntry + comments?: Map + commentsVisibilityAtLineNumber?: Map } -const { scheduleTask: scheduleLowPriorityTask, cancelTask } = createRequestIdleCallbackTaskPool() +export interface CommentRestorationTrackingState extends DiffCommentItem { + uncommittedText?: string + showReplyPlaceHolder?: boolean + uncommittedEditComments?: Map +} + +const { scheduleTask, cancelTask } = createRequestAnimationFrameTaskPool() export const DiffViewer = React.memo(DiffViewerInternal) diff --git a/web/src/components/DiffViewer/InViewDiffBlockRenderer.tsx b/web/src/components/DiffViewer/InViewDiffBlockRenderer.tsx index ccb1d3383..5b75eecfb 100644 --- a/web/src/components/DiffViewer/InViewDiffBlockRenderer.tsx +++ b/web/src/components/DiffViewer/InViewDiffBlockRenderer.tsx @@ -19,7 +19,6 @@ import cx from 'classnames' import { Container, useIsMounted } from '@harnessio/uicore' import { useInView } from 'react-intersection-observer' import type { FCWithChildren } from 'utils/types' -import Config from 'Config' import { useResizeObserver } from 'hooks/useResizeObserver' import css from './InViewDiffBlockRenderer.module.scss' @@ -27,19 +26,21 @@ interface InViewDiffBlockRendererProps { root: RefObject blockName: string shouldRetainChildren: (containerDOM: HTMLElement | null) => boolean + detectionMargin: number } const InViewDiffBlockRendererInternal: FCWithChildren = ({ root, blockName, children, - shouldRetainChildren + shouldRetainChildren, + detectionMargin }) => { const containerRef = useRef(null) const isMounted = useIsMounted() const { ref, inView } = useInView({ root: root.current, - rootMargin: `${Config.IN_VIEWPORT_DETECTION_MARGIN}px 0px ${Config.IN_VIEWPORT_DETECTION_MARGIN}px 0px`, + rootMargin: `${detectionMargin}px 0px ${detectionMargin}px 0px`, initialInView: false }) const setContainerRef = useCallback( diff --git a/web/src/components/DiffViewer/usePullReqComments.tsx b/web/src/components/DiffViewer/usePullReqComments.tsx index a859b666e..c0f7b7b91 100644 --- a/web/src/components/DiffViewer/usePullReqComments.tsx +++ b/web/src/components/DiffViewer/usePullReqComments.tsx @@ -14,7 +14,7 @@ * limitations under the License. */ -import React, { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react' +import React, { useCallback, useEffect, useLayoutEffect, useMemo, useRef } from 'react' import { useMutate } from 'restful-react' import Selecto from 'selecto' import ReactDOM from 'react-dom' @@ -35,6 +35,7 @@ import { CodeCommentStatusSelect } from 'components/CodeCommentStatusSelect/Code import { dispatchCustomEvent } from 'hooks/useEventListener' import { UseGetPullRequestInfoResult, usePullReqActivities } from 'pages/PullRequest/useGetPullRequestInfo' import { CommentThreadTopDecoration } from 'components/CommentThreadTopDecoration/CommentThreadTopDecoration' +import type { DiffViewerExchangeState } from './DiffViewer' import { activitiesToDiffCommentItems, activityToCommentItem, @@ -68,6 +69,7 @@ interface UsePullReqCommentsProps extends Pick { contentRef: React.RefObject refetchActivities?: UseGetPullRequestInfoResult['refetchActivities'] setDirty?: React.Dispatch> + memorizedState: Map } export function usePullReqComments({ @@ -85,7 +87,8 @@ export function usePullReqComments({ containerRef, contentRef, refetchActivities, - setDirty + setDirty, + memorizedState }: UsePullReqCommentsProps) { const activities = usePullReqActivities() const { getString } = useStrings() @@ -98,7 +101,18 @@ export function usePullReqComments({ ) const { save, update, remove } = useCommentAPI(commentPath) const location = useLocation() - const [comments] = useState(new Map>()) + + const comments = useMemo(() => { + let _comments = memorizedState.get(diff.filePath)?.comments + + if (!_comments) { + _comments = new Map>() + memorizedState.set(diff.filePath, { ...memorizedState.get(diff.filePath), comments: _comments }) + } + + return _comments + }, [diff.filePath, memorizedState]) + const copyLinkToComment = useCallback( (id, commentItem) => { const path = `${routes.toCODEPullRequest({ @@ -252,8 +266,18 @@ export function usePullReqComments({ commentRowElement.innerHTML = `` lineInfo.rowElement.after(commentRowElement) + if (!memorizedState.get(diff.filePath)?.commentsVisibilityAtLineNumber) { + memorizedState.set(diff.filePath, { + ...memorizedState.get(diff.filePath), + commentsVisibilityAtLineNumber: new Map() + }) + } + + // Get show commments from memorizedState + const showComments = memorizedState.get(diff.filePath)?.commentsVisibilityAtLineNumber?.get(comment.lineNumberEnd) + // Set both place-holder and comment box hidden when comment thread is resolved - if (isCommentThreadResolved) { + if ((showComments === undefined && isCommentThreadResolved) || showComments === false) { oppositeRowPlaceHolder.setAttribute('hidden', '') commentRowElement.setAttribute('hidden', '') } @@ -300,6 +324,12 @@ export function usePullReqComments({ lang: filenameToLanguage(diff.filePath.split('/').pop()) } + // Vars to verified if a CommentBox is restored from innerHTML/textContent optimization + const _memorizedState = memorizedState.get(diff.filePath)?.comments?.get(commentThreadId) + const isCommentBoxRestored = + (commentThreadId && commentThreadId < 0 && _memorizedState?.uncommittedText !== undefined) || + _memorizedState?.showReplyPlaceHolder === false + // Note: CommentBox is rendered as an independent React component. // Everything passed to it must be either values, or refs. // If you pass callbacks or states, they won't be updated and @@ -311,6 +341,7 @@ export function usePullReqComments({ routingId={routingId} standalone={standalone} repoMetadata={repoMetadata} + commentThreadId={commentThreadId} commentItems={comment._commentItems as CommentItem[]} initialContent={''} width={getCommentBoxWidth(isSideBySide)} @@ -323,7 +354,7 @@ export function usePullReqComments({ last.style.height = `${boxHeight}px` } }} - autoFocusAndPosition={true} + autoFocusAndPosition={isCommentBoxRestored ? false : true} enableReplyPlaceHolder={(comment._commentItems as CommentItem[])?.length > 0} onCancel={comment.destroy} setDirty={setDirty || noop} @@ -470,6 +501,8 @@ export function usePullReqComments({ /> ) }} + memorizedState={_memorizedState} + commentsVisibilityAtLineNumber={memorizedState.get(diff.filePath)?.commentsVisibilityAtLineNumber} /> , element @@ -498,7 +531,8 @@ export function usePullReqComments({ refetchActivities, copyLinkToComment, markSelectedLines, - updateDataCommentIds + updateDataCommentIds, + memorizedState ] ) diff --git a/web/src/hooks/useIsSidebarExpanded.ts b/web/src/hooks/useIsSidebarExpanded.ts index d4f37939b..30eb84bd0 100644 --- a/web/src/hooks/useIsSidebarExpanded.ts +++ b/web/src/hooks/useIsSidebarExpanded.ts @@ -49,7 +49,7 @@ export function useCollapseHarnessNav() { if (handled.current) { const nav = document.getElementById('main-side-nav') const pullReqNavItem = nav?.querySelector('[data-code-repo-section="pull-requests"]') - const toggleNavButton = nav?.querySelector('span[icon][class*="SideNavToggleButton"]') as HTMLElement + const toggleNavButton = nav?.querySelector('span[icon][class*="icon-symbol-triangle"]') as HTMLElement if (pullReqNavItem && toggleNavButton) { const isCollapsed = pullReqNavItem.clientWidth <= 64 @@ -63,7 +63,7 @@ export function useCollapseHarnessNav() { } return () => { - if (handled.current) { + if (handled.current && toggleNavButton) { toggleNavButton.click() } }