From 4313d4c7ab673e43a0ef335c8548df45c245e8f9 Mon Sep 17 00:00:00 2001 From: kiwina Date: Mon, 2 Jun 2025 15:48:28 +0800 Subject: [PATCH 1/2] fix: address multiple memory leaks in CodeBlock component (CodeBlock_247, CodeBlock_459, CodeBlock_694) --- .../src/components/common/CodeBlock.tsx | 84 +++++++++++++++---- 1 file changed, 68 insertions(+), 16 deletions(-) diff --git a/webview-ui/src/components/common/CodeBlock.tsx b/webview-ui/src/components/common/CodeBlock.tsx index da3eb6429c..17506c84b0 100644 --- a/webview-ui/src/components/common/CodeBlock.tsx +++ b/webview-ui/src/components/common/CodeBlock.tsx @@ -232,6 +232,10 @@ const CodeBlock = memo( const copyButtonWrapperRef = useRef(null) const { showCopyFeedback, copyWithFeedback } = useCopyToClipboard() const { t } = useAppTranslation() + const isMountedRef = useRef(true) + const buttonPositionTimeoutRef = useRef(null) + const collapseTimeout1Ref = useRef(null) + const collapseTimeout2Ref = useRef(null) // Update current language when prop changes, but only if user hasn't // made a selection. @@ -243,6 +247,26 @@ const CodeBlock = memo( } }, [language, currentLanguage]) + // Manage mounted state + useEffect(() => { + isMountedRef.current = true + return () => { + isMountedRef.current = false + } + }, []) + + // Cleanup for collapse/expand timeouts + useEffect(() => { + return () => { + if (collapseTimeout1Ref.current) { + clearTimeout(collapseTimeout1Ref.current) + } + if (collapseTimeout2Ref.current) { + clearTimeout(collapseTimeout2Ref.current) + } + } + }, []) + // Syntax highlighting with cached Shiki instance. useEffect(() => { const fallback = `
${source || ""}
` @@ -250,10 +274,13 @@ const CodeBlock = memo( const highlight = async () => { // Show plain text if language needs to be loaded. if (currentLanguage && !isLanguageLoaded(currentLanguage)) { - setHighlightedCode(fallback) + if (isMountedRef.current) { + setHighlightedCode(fallback) + } } const highlighter = await getHighlighter(currentLanguage) + if (!isMountedRef.current) return const html = await highlighter.codeToHtml(source || "", { lang: currentLanguage || "txt", @@ -277,13 +304,18 @@ const CodeBlock = memo( }, ] as ShikiTransformer[], }) + if (!isMountedRef.current) return - setHighlightedCode(html) + if (isMountedRef.current) { + setHighlightedCode(html) + } } highlight().catch((e) => { console.error("[CodeBlock] Syntax highlighting error:", e, "\nStack trace:", e.stack) - setHighlightedCode(fallback) + if (isMountedRef.current) { + setHighlightedCode(fallback) + } }) }, [source, currentLanguage, collapsedHeight]) @@ -455,8 +487,15 @@ const CodeBlock = memo( // Update button position and scroll when highlightedCode changes useEffect(() => { if (highlightedCode) { + // Clear any existing timeout before setting a new one + if (buttonPositionTimeoutRef.current) { + clearTimeout(buttonPositionTimeoutRef.current) + } // Update button position - setTimeout(updateCodeBlockButtonPosition, 0) + buttonPositionTimeoutRef.current = setTimeout(() => { + updateCodeBlockButtonPosition() + buttonPositionTimeoutRef.current = null // Optional: Clear ref after execution + }, 0) // Scroll to bottom if needed (immediately after Shiki updates) if (shouldScrollAfterHighlightRef.current) { @@ -479,6 +518,12 @@ const CodeBlock = memo( shouldScrollAfterHighlightRef.current = false } } + // Cleanup function for this effect + return () => { + if (buttonPositionTimeoutRef.current) { + clearTimeout(buttonPositionTimeoutRef.current) + } + } }, [highlightedCode, updateCodeBlockButtonPosition]) // Advanced inertial scroll chaining @@ -682,23 +727,30 @@ const CodeBlock = memo( {showCollapseButton && ( { - // Get the current code block element and scrollable container - const codeBlock = codeBlockRef.current - const scrollContainer = document.querySelector('[data-virtuoso-scroller="true"]') - if (!codeBlock || !scrollContainer) return - + // Get the current code block element + const codeBlock = codeBlockRef.current // Capture ref early // Toggle window shade state setWindowShade(!windowShade) + // Clear any previous timeouts + if (collapseTimeout1Ref.current) clearTimeout(collapseTimeout1Ref.current) + if (collapseTimeout2Ref.current) clearTimeout(collapseTimeout2Ref.current) + // After UI updates, ensure code block is visible and update button position - setTimeout( + collapseTimeout1Ref.current = setTimeout( () => { - codeBlock.scrollIntoView({ behavior: "smooth", block: "nearest" }) - - // Wait for scroll to complete before updating button position - setTimeout(() => { - updateCodeBlockButtonPosition() - }, 50) + if (codeBlock) { + // Check if codeBlock element still exists + codeBlock.scrollIntoView({ behavior: "smooth", block: "nearest" }) + + // Wait for scroll to complete before updating button position + collapseTimeout2Ref.current = setTimeout(() => { + // updateCodeBlockButtonPosition itself should also check for refs if needed + updateCodeBlockButtonPosition() + collapseTimeout2Ref.current = null + }, 50) + } + collapseTimeout1Ref.current = null }, WINDOW_SHADE_SETTINGS.transitionDelayS * 1000 + 50, ) From 1e3012285b9e8e5515f235ddead7387fb5549d50 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Mon, 23 Jun 2025 17:22:52 -0500 Subject: [PATCH 2/2] fix: Address review feedback for CodeBlock memory leak fixes - Consolidate isMountedRef management into syntax highlighting useEffect - Unify timeout cleanup patterns for consistent maintainability - Maintain all existing memory leak protections - Improve code organization and readability Addresses review comments from daniel-lxs in PR #4244 --- .../src/components/common/CodeBlock.tsx | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/webview-ui/src/components/common/CodeBlock.tsx b/webview-ui/src/components/common/CodeBlock.tsx index 17506c84b0..06f929b830 100644 --- a/webview-ui/src/components/common/CodeBlock.tsx +++ b/webview-ui/src/components/common/CodeBlock.tsx @@ -247,28 +247,11 @@ const CodeBlock = memo( } }, [language, currentLanguage]) - // Manage mounted state + // Syntax highlighting with cached Shiki instance and mounted state management useEffect(() => { + // Set mounted state at the beginning of this effect isMountedRef.current = true - return () => { - isMountedRef.current = false - } - }, []) - - // Cleanup for collapse/expand timeouts - useEffect(() => { - return () => { - if (collapseTimeout1Ref.current) { - clearTimeout(collapseTimeout1Ref.current) - } - if (collapseTimeout2Ref.current) { - clearTimeout(collapseTimeout2Ref.current) - } - } - }, []) - // Syntax highlighting with cached Shiki instance. - useEffect(() => { const fallback = `
${source || ""}
` const highlight = async () => { @@ -317,6 +300,23 @@ const CodeBlock = memo( setHighlightedCode(fallback) } }) + + // Cleanup function - manage mounted state and clear all timeouts + return () => { + isMountedRef.current = false + if (buttonPositionTimeoutRef.current) { + clearTimeout(buttonPositionTimeoutRef.current) + buttonPositionTimeoutRef.current = null + } + if (collapseTimeout1Ref.current) { + clearTimeout(collapseTimeout1Ref.current) + collapseTimeout1Ref.current = null + } + if (collapseTimeout2Ref.current) { + clearTimeout(collapseTimeout2Ref.current) + collapseTimeout2Ref.current = null + } + } }, [source, currentLanguage, collapsedHeight]) // Check if content height exceeds collapsed height whenever content changes