From e6dc4c4fc92f312b47a39a90effe988a21a49dd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Ctan-nhu=E2=80=9D?= <“tan@harness.io”> Date: Fri, 14 Jun 2024 17:40:21 -0700 Subject: [PATCH] Render off-screen diffs in a single pre tag to boost performance --- web/src/Config.ts | 2 +- web/src/components/CommentBox/CommentBox.tsx | 4 +- .../CommentBox/comment-resolved.svg | 2 +- .../DiffViewer/DiffViewer.module.scss | 14 +- .../DiffViewer/DiffViewer.module.scss.d.ts | 2 +- web/src/components/DiffViewer/DiffViewer.tsx | 133 ++++++++++++------ 6 files changed, 104 insertions(+), 53 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/CommentBox/CommentBox.tsx b/web/src/components/CommentBox/CommentBox.tsx index 3a9cd66e6..49e4a28d1 100644 --- a/web/src/components/CommentBox/CommentBox.tsx +++ b/web/src/components/CommentBox/CommentBox.tsx @@ -363,8 +363,6 @@ const CommentsThread = ({ 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,6 +374,7 @@ const CommentsThread = ({ oppositePlaceHolders?.forEach(dom => toggleHidden(dom)) + toggleHidden(commentRow) commentRow = commentRow.nextElementSibling as HTMLElement } show.current = !show.current @@ -388,6 +387,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) 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..c026a4f96 100644 --- a/web/src/components/DiffViewer/DiffViewer.tsx +++ b/web/src/components/DiffViewer/DiffViewer.tsx @@ -46,10 +46,10 @@ 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' @@ -165,6 +165,7 @@ const DiffViewerInternal: React.FC = ({ }, [ref] ) + const contentHTML = useRef(null) useResizeObserver( contentRef, @@ -178,20 +179,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... @@ -290,7 +277,7 @@ const DiffViewerInternal: React.FC = ({ if (isInViewport(containerRef.current as Element, 1000)) { renderDiffAndComments() } else { - taskId = scheduleLowPriorityTask(renderDiffAndComments) + taskId = scheduleTask(renderDiffAndComments) } } @@ -316,6 +303,67 @@ 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 + + // TODO: Might be good to clean textContent a bit to not include + // diff header info, line numbers, hunk headers, etc... + + // Set innerHTML to a pre tag with the same height to avoid reflow + // The pre textContent allows Cmd/Ctrl-F to work + dom.innerHTML = `
${textContent}
` + } + } + }, + [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( @@ -505,28 +553,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 +587,7 @@ const DiffViewerInternal: React.FC = ({ } const BLOCK_HEIGHT = '--block-height' +const ACTION_SHOW_DIFF = 'showDiff' export enum DiffViewerEvent { SCROLL_INTO_VIEW = 'scrollIntoView' @@ -551,6 +604,6 @@ export interface DiffViewerExchangeState { fullDiff?: DiffFileEntry } -const { scheduleTask: scheduleLowPriorityTask, cancelTask } = createRequestIdleCallbackTaskPool() +const { scheduleTask, cancelTask } = createRequestAnimationFrameTaskPool() export const DiffViewer = React.memo(DiffViewerInternal)