Skip to content

Commit b8843db

Browse files
kiwinadaniel-lxs
authored andcommitted
fix: Address multiple memory leaks in CodeBlock component (#4244)
* fix: address multiple memory leaks in CodeBlock component (CodeBlock_247, CodeBlock_459, CodeBlock_694) * 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 --------- Co-authored-by: Daniel Riccio <[email protected]>
1 parent 2516c1f commit b8843db

File tree

1 file changed

+69
-17
lines changed

1 file changed

+69
-17
lines changed

webview-ui/src/components/common/CodeBlock.tsx

Lines changed: 69 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,10 @@ const CodeBlock = memo(
232232
const copyButtonWrapperRef = useRef<HTMLDivElement>(null)
233233
const { showCopyFeedback, copyWithFeedback } = useCopyToClipboard()
234234
const { t } = useAppTranslation()
235+
const isMountedRef = useRef(true)
236+
const buttonPositionTimeoutRef = useRef<NodeJS.Timeout | null>(null)
237+
const collapseTimeout1Ref = useRef<NodeJS.Timeout | null>(null)
238+
const collapseTimeout2Ref = useRef<NodeJS.Timeout | null>(null)
235239

236240
// Update current language when prop changes, but only if user hasn't
237241
// made a selection.
@@ -243,17 +247,23 @@ const CodeBlock = memo(
243247
}
244248
}, [language, currentLanguage])
245249

246-
// Syntax highlighting with cached Shiki instance.
250+
// Syntax highlighting with cached Shiki instance and mounted state management
247251
useEffect(() => {
252+
// Set mounted state at the beginning of this effect
253+
isMountedRef.current = true
254+
248255
const fallback = `<pre style="padding: 0; margin: 0;"><code class="hljs language-${currentLanguage || "txt"}">${source || ""}</code></pre>`
249256

250257
const highlight = async () => {
251258
// Show plain text if language needs to be loaded.
252259
if (currentLanguage && !isLanguageLoaded(currentLanguage)) {
253-
setHighlightedCode(fallback)
260+
if (isMountedRef.current) {
261+
setHighlightedCode(fallback)
262+
}
254263
}
255264

256265
const highlighter = await getHighlighter(currentLanguage)
266+
if (!isMountedRef.current) return
257267

258268
const html = await highlighter.codeToHtml(source || "", {
259269
lang: currentLanguage || "txt",
@@ -277,14 +287,36 @@ const CodeBlock = memo(
277287
},
278288
] as ShikiTransformer[],
279289
})
290+
if (!isMountedRef.current) return
280291

281-
setHighlightedCode(html)
292+
if (isMountedRef.current) {
293+
setHighlightedCode(html)
294+
}
282295
}
283296

284297
highlight().catch((e) => {
285298
console.error("[CodeBlock] Syntax highlighting error:", e, "\nStack trace:", e.stack)
286-
setHighlightedCode(fallback)
299+
if (isMountedRef.current) {
300+
setHighlightedCode(fallback)
301+
}
287302
})
303+
304+
// Cleanup function - manage mounted state and clear all timeouts
305+
return () => {
306+
isMountedRef.current = false
307+
if (buttonPositionTimeoutRef.current) {
308+
clearTimeout(buttonPositionTimeoutRef.current)
309+
buttonPositionTimeoutRef.current = null
310+
}
311+
if (collapseTimeout1Ref.current) {
312+
clearTimeout(collapseTimeout1Ref.current)
313+
collapseTimeout1Ref.current = null
314+
}
315+
if (collapseTimeout2Ref.current) {
316+
clearTimeout(collapseTimeout2Ref.current)
317+
collapseTimeout2Ref.current = null
318+
}
319+
}
288320
}, [source, currentLanguage, collapsedHeight])
289321

290322
// Check if content height exceeds collapsed height whenever content changes
@@ -455,8 +487,15 @@ const CodeBlock = memo(
455487
// Update button position and scroll when highlightedCode changes
456488
useEffect(() => {
457489
if (highlightedCode) {
490+
// Clear any existing timeout before setting a new one
491+
if (buttonPositionTimeoutRef.current) {
492+
clearTimeout(buttonPositionTimeoutRef.current)
493+
}
458494
// Update button position
459-
setTimeout(updateCodeBlockButtonPosition, 0)
495+
buttonPositionTimeoutRef.current = setTimeout(() => {
496+
updateCodeBlockButtonPosition()
497+
buttonPositionTimeoutRef.current = null // Optional: Clear ref after execution
498+
}, 0)
460499

461500
// Scroll to bottom if needed (immediately after Shiki updates)
462501
if (shouldScrollAfterHighlightRef.current) {
@@ -479,6 +518,12 @@ const CodeBlock = memo(
479518
shouldScrollAfterHighlightRef.current = false
480519
}
481520
}
521+
// Cleanup function for this effect
522+
return () => {
523+
if (buttonPositionTimeoutRef.current) {
524+
clearTimeout(buttonPositionTimeoutRef.current)
525+
}
526+
}
482527
}, [highlightedCode, updateCodeBlockButtonPosition])
483528

484529
// Advanced inertial scroll chaining
@@ -682,23 +727,30 @@ const CodeBlock = memo(
682727
{showCollapseButton && (
683728
<CodeBlockButton
684729
onClick={() => {
685-
// Get the current code block element and scrollable container
686-
const codeBlock = codeBlockRef.current
687-
const scrollContainer = document.querySelector('[data-virtuoso-scroller="true"]')
688-
if (!codeBlock || !scrollContainer) return
689-
730+
// Get the current code block element
731+
const codeBlock = codeBlockRef.current // Capture ref early
690732
// Toggle window shade state
691733
setWindowShade(!windowShade)
692734

735+
// Clear any previous timeouts
736+
if (collapseTimeout1Ref.current) clearTimeout(collapseTimeout1Ref.current)
737+
if (collapseTimeout2Ref.current) clearTimeout(collapseTimeout2Ref.current)
738+
693739
// After UI updates, ensure code block is visible and update button position
694-
setTimeout(
740+
collapseTimeout1Ref.current = setTimeout(
695741
() => {
696-
codeBlock.scrollIntoView({ behavior: "smooth", block: "nearest" })
697-
698-
// Wait for scroll to complete before updating button position
699-
setTimeout(() => {
700-
updateCodeBlockButtonPosition()
701-
}, 50)
742+
if (codeBlock) {
743+
// Check if codeBlock element still exists
744+
codeBlock.scrollIntoView({ behavior: "smooth", block: "nearest" })
745+
746+
// Wait for scroll to complete before updating button position
747+
collapseTimeout2Ref.current = setTimeout(() => {
748+
// updateCodeBlockButtonPosition itself should also check for refs if needed
749+
updateCodeBlockButtonPosition()
750+
collapseTimeout2Ref.current = null
751+
}, 50)
752+
}
753+
collapseTimeout1Ref.current = null
702754
},
703755
WINDOW_SHADE_SETTINGS.transitionDelayS * 1000 + 50,
704756
)

0 commit comments

Comments
 (0)