From abed6dcd275206ac84c32a7ab158d024d610cb57 Mon Sep 17 00:00:00 2001 From: sam hoang Date: Tue, 27 May 2025 20:38:10 +0700 Subject: [PATCH 1/2] fix(webview): resolve memory leak in ChatView by stabilizing callback props - Stabilize handleSendMessage using clineAskRef to prevent frequent re-creation - Stabilize toggleRowExpansion by extracting handleSetExpandedRow and managing dependencies - Re-integrate scrolling logic into useEffect hook to avoid destabilizing callbacks - Add everVisibleMessagesTsRef to reduce unnecessary ChatRow remounts by Virtuoso - Update onToggleExpand signature to accept timestamp parameter for better stability - Remove diagnostic console.log statements used for debugging callback changes These changes address detached DOM elements memory leak caused by frequent callback re-creation triggering unnecessary component re-renders and preventing proper garbage collection of chat message DOM nodes. --- pnpm-lock.yaml | 3 + webview-ui/package.json | 3 +- webview-ui/src/components/chat/ChatRow.tsx | 37 ++-- webview-ui/src/components/chat/ChatView.tsx | 214 ++++++++++++-------- 4 files changed, 151 insertions(+), 106 deletions(-) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index d7fc259cab..9340d50d2f 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -577,6 +577,9 @@ importers: knuth-shuffle-seeded: specifier: ^1.0.6 version: 1.0.6 + lru-cache: + specifier: ^11.1.0 + version: 11.1.0 lucide-react: specifier: ^0.510.0 version: 0.510.0(react@18.3.1) diff --git a/webview-ui/package.json b/webview-ui/package.json index 269b8e6e49..9a31a423f0 100644 --- a/webview-ui/package.json +++ b/webview-ui/package.json @@ -47,6 +47,7 @@ "i18next": "^24.2.2", "i18next-http-backend": "^3.0.2", "knuth-shuffle-seeded": "^1.0.6", + "lru-cache": "^11.1.0", "lucide-react": "^0.510.0", "mermaid": "^11.4.1", "posthog-js": "^1.227.2", @@ -70,8 +71,8 @@ "tailwindcss": "^4.0.0", "tailwindcss-animate": "^1.0.7", "unist-util-visit": "^5.0.0", - "vscode-material-icons": "^0.1.1", "use-sound": "^5.0.0", + "vscode-material-icons": "^0.1.1", "vscrui": "^0.2.2", "zod": "^3.24.2" }, diff --git a/webview-ui/src/components/chat/ChatRow.tsx b/webview-ui/src/components/chat/ChatRow.tsx index e6b8bb601a..eaabb77e70 100644 --- a/webview-ui/src/components/chat/ChatRow.tsx +++ b/webview-ui/src/components/chat/ChatRow.tsx @@ -1,4 +1,4 @@ -import React, { memo, useEffect, useMemo, useRef, useState } from "react" +import React, { memo, useCallback, useEffect, useMemo, useRef, useState } from "react" import { useSize } from "react-use" import { useTranslation, Trans } from "react-i18next" import deepEqual from "fast-deep-equal" @@ -44,7 +44,7 @@ interface ChatRowProps { isExpanded: boolean isLast: boolean isStreaming: boolean - onToggleExpand: () => void + onToggleExpand: (ts: number) => void onHeightChange: (isTaller: boolean) => void onSuggestionClick?: (answer: string, event?: React.MouseEvent) => void } @@ -103,6 +103,11 @@ export const ChatRowContent = ({ const [showCopySuccess, setShowCopySuccess] = useState(false) const { copyWithFeedback } = useCopyToClipboard() + // Memoized callback to prevent re-renders caused by inline arrow functions + const handleToggleExpand = useCallback(() => { + onToggleExpand(message.ts) + }, [onToggleExpand, message.ts]) + const [cost, apiReqCancelReason, apiReqStreamingFailedMessage] = useMemo(() => { if (message.text !== null && message.text !== undefined && message.say === "api_req_started") { const info = safeJsonParse(message.text) @@ -302,7 +307,7 @@ export const ChatRowContent = ({ progressStatus={message.progressStatus} isLoading={message.partial} isExpanded={isExpanded} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} /> ) @@ -328,7 +333,7 @@ export const ChatRowContent = ({ progressStatus={message.progressStatus} isLoading={message.partial} isExpanded={isExpanded} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} /> ) @@ -350,7 +355,7 @@ export const ChatRowContent = ({ progressStatus={message.progressStatus} isLoading={message.partial} isExpanded={isExpanded} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} /> ) @@ -389,7 +394,7 @@ export const ChatRowContent = ({ language={getLanguageFromPath(tool.path || "") || "log"} isLoading={message.partial} isExpanded={isExpanded} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} /> ) @@ -435,7 +440,7 @@ export const ChatRowContent = ({ language="markdown" isLoading={message.partial} isExpanded={isExpanded} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} /> ) @@ -455,7 +460,7 @@ export const ChatRowContent = ({ code={tool.content} language="shell-session" isExpanded={isExpanded} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} /> ) @@ -475,7 +480,7 @@ export const ChatRowContent = ({ code={tool.content} language="shellsession" isExpanded={isExpanded} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} /> ) @@ -495,7 +500,7 @@ export const ChatRowContent = ({ code={tool.content} language="markdown" isExpanded={isExpanded} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} /> ) @@ -525,7 +530,7 @@ export const ChatRowContent = ({ code={tool.content} language="shellsession" isExpanded={isExpanded} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} /> ) @@ -813,7 +818,7 @@ export const ChatRowContent = ({ MozUserSelect: "none", msUserSelect: "none", }} - onClick={onToggleExpand}> + onClick={handleToggleExpand}>
{icon} {title} @@ -852,7 +857,7 @@ export const ChatRowContent = ({ code={safeJsonParse(message.text)?.request} language="markdown" isExpanded={true} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} />
)} @@ -898,7 +903,7 @@ export const ChatRowContent = ({ language="diff" isFeedback={true} isExpanded={isExpanded} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} /> ) @@ -945,7 +950,7 @@ export const ChatRowContent = ({ code={message.text} language="json" isExpanded={true} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} /> @@ -1105,7 +1110,7 @@ export const ChatRowContent = ({ code={useMcpServer.arguments} language="json" isExpanded={true} - onToggleExpand={onToggleExpand} + onToggleExpand={handleToggleExpand} /> )} diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index 6eaceb1374..067d7b549a 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -38,6 +38,7 @@ import TaskHeader from "./TaskHeader" import AutoApproveMenu from "./AutoApproveMenu" import SystemPromptWarning from "./SystemPromptWarning" import { CheckpointWarning } from "./CheckpointWarning" +import { LRUCache } from "lru-cache" export interface ChatViewProps { isHidden: boolean @@ -91,6 +92,11 @@ const ChatViewComponent: React.ForwardRefRenderFunction { + messagesRef.current = messages + }, [messages]) + const { tasks } = useTaskSearch() // Initialize expanded state based on the persisted setting (default to expanded if undefined) @@ -128,6 +134,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction(null) const [expandedRows, setExpandedRows] = useState>({}) + const prevExpandedRowsRef = useRef>() const scrollContainerRef = useRef(null) const disableAutoScrollRef = useRef(false) const [showScrollToBottom, setShowScrollToBottom] = useState(false) @@ -136,6 +143,17 @@ const ChatViewComponent: React.ForwardRefRenderFunction(false) const [showCheckpointWarning, setShowCheckpointWarning] = useState(false) const [isCondensing, setIsCondensing] = useState(false) + const everVisibleMessagesTsRef = useRef>( + new LRUCache({ + max: 250, + ttl: 1000 * 60 * 15, // 1 hour TTL for long-running tasks + }), + ) + + const clineAskRef = useRef(clineAsk) + useEffect(() => { + clineAskRef.current = clineAsk + }, [clineAsk]) // UI layout depends on the last 2 messages // (since it relies on the content of these messages, we are deep comparing. i.e. the button state after hitting button sets enableButtons to false, and this effect otherwise would have to true again even if messages didn't change @@ -367,7 +385,32 @@ const ChatViewComponent: React.ForwardRefRenderFunction setExpandedRows({}), [task?.ts]) + useEffect(() => { + setExpandedRows({}) + everVisibleMessagesTsRef.current.clear() // Clear for new task + }, [task?.ts]) + + useEffect(() => () => everVisibleMessagesTsRef.current.clear(), []) + + useEffect(() => { + const prev = prevExpandedRowsRef.current + let wasAnyRowExpandedByUser = false + if (prev) { + // Check if any row transitioned from false/undefined to true + for (const [tsKey, isExpanded] of Object.entries(expandedRows)) { + const ts = Number(tsKey) + if (isExpanded && !(prev[ts] ?? false)) { + wasAnyRowExpandedByUser = true + break + } + } + } + + if (wasAnyRowExpandedByUser) { + disableAutoScrollRef.current = true + } + prevExpandedRowsRef.current = expandedRows // Store current state for next comparison + }, [expandedRows]) const isStreaming = useMemo(() => { // Checking clineAsk isn't enough since messages effect may be called @@ -428,10 +471,13 @@ const ChatViewComponent: React.ForwardRefRenderFunction 0) { - if (messages.length === 0) { + if (messagesRef.current.length === 0) { vscode.postMessage({ type: "newTask", text, images }) - } else if (clineAsk) { - switch (clineAsk) { + } else if (clineAskRef.current) { + // Use clineAskRef.current + switch ( + clineAskRef.current // Use clineAskRef.current + ) { case "followup": case "tool": case "browser_action_launch": @@ -451,7 +497,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction { - return modifiedMessages.filter((message) => { + const newVisibleMessages = modifiedMessages.filter((message) => { + if (everVisibleMessagesTsRef.current.has(message.ts)) { + // If it was ever visible, and it's not one of the types that should always be hidden once processed, keep it. + // This helps prevent flickering for messages like 'api_req_retry_delayed' if they are no longer the absolute last. + const alwaysHiddenOnceProcessedAsk: ClineAsk[] = [ + "api_req_failed", + "resume_task", + "resume_completed_task", + ] + const alwaysHiddenOnceProcessedSay = [ + "api_req_finished", + "api_req_retried", + "api_req_deleted", + "mcp_server_request_started", + ] + if (message.ask && alwaysHiddenOnceProcessedAsk.includes(message.ask)) return false + if (message.say && alwaysHiddenOnceProcessedSay.includes(message.say)) return false + // Also, re-evaluate empty text messages if they were previously visible but now empty (e.g. partial stream ended) + if (message.say === "text" && (message.text ?? "") === "" && (message.images?.length ?? 0) === 0) { + return false + } + return true + } + + // Original filter logic switch (message.ask) { case "completion_result": - // Don't show a chat row for a completion_result ask without - // text. This specific type of message only occurs if cline - // wants to execute a command as part of its completion - // result, in which case we interject the completion_result - // tool with the execute_command tool. - if (message.text === "") { - return false - } + if (message.text === "") return false break - case "api_req_failed": // This message is used to update the latest `api_req_started` that the request failed. + case "api_req_failed": case "resume_task": case "resume_completed_task": return false } switch (message.say) { - case "api_req_finished": // `combineApiRequests` removes this from `modifiedMessages` anyways. - case "api_req_retried": // This message is used to update the latest `api_req_started` that the request was retried. - case "api_req_deleted": // Aggregated `api_req` metrics from deleted messages. + case "api_req_finished": + case "api_req_retried": + case "api_req_deleted": return false case "api_req_retry_delayed": - // Only show the retry message if it's the last message or - // the last messages is api_req_retry_delayed+resume_task. const last1 = modifiedMessages.at(-1) const last2 = modifiedMessages.at(-2) if (last1?.ask === "resume_task" && last2 === message) { - return true - } - return message === last1 - case "text": - // Sometimes cline returns an empty text message, we don't - // want to render these. (We also use a say text for user - // messages, so in case they just sent images we still - // render that.) - if ((message.text ?? "") === "" && (message.images?.length ?? 0) === 0) { + // This specific sequence should be visible + } else if (message !== last1) { + // If not the specific sequence above, and not the last message, hide it. return false } break + case "text": + if ((message.text ?? "") === "" && (message.images?.length ?? 0) === 0) return false + break case "mcp_server_request_started": return false } return true }) + + // Update the set of ever-visible messages (LRUCache automatically handles cleanup) + newVisibleMessages.forEach((msg) => everVisibleMessagesTsRef.current.set(msg.ts, true)) + + return newVisibleMessages }, [modifiedMessages]) const isReadOnlyToolAction = useCallback((message: ClineMessage | undefined) => { @@ -1006,54 +1069,21 @@ const ChatViewComponent: React.ForwardRefRenderFunction { + setExpandedRows((prev) => ({ ...prev, [ts]: expand === undefined ? !prev[ts] : expand })) + }, + [setExpandedRows], // setExpandedRows is stable + ) + // Scroll when user toggles certain rows. const toggleRowExpansion = useCallback( (ts: number) => { - const isCollapsing = expandedRows[ts] ?? false - const lastGroup = groupedMessages.at(-1) - const isLast = Array.isArray(lastGroup) ? lastGroup[0].ts === ts : lastGroup?.ts === ts - const secondToLastGroup = groupedMessages.at(-2) - const isSecondToLast = Array.isArray(secondToLastGroup) - ? secondToLastGroup[0].ts === ts - : secondToLastGroup?.ts === ts - - const isLastCollapsedApiReq = - isLast && - !Array.isArray(lastGroup) && // Make sure it's not a browser session group - lastGroup?.say === "api_req_started" && - !expandedRows[lastGroup.ts] - - setExpandedRows((prev) => ({ ...prev, [ts]: !prev[ts] })) - - // Disable auto scroll when user expands row - if (!isCollapsing) { - disableAutoScrollRef.current = true - } - - if (isCollapsing && isAtBottom) { - const timer = setTimeout(() => scrollToBottomAuto(), 0) - return () => clearTimeout(timer) - } else if (isLast || isSecondToLast) { - if (isCollapsing) { - if (isSecondToLast && !isLastCollapsedApiReq) { - return - } - - const timer = setTimeout(() => scrollToBottomAuto(), 0) - return () => clearTimeout(timer) - } else { - const timer = setTimeout(() => { - virtuosoRef.current?.scrollToIndex({ - index: groupedMessages.length - (isLast ? 1 : 2), - align: "start", - }) - }, 0) - - return () => clearTimeout(timer) - } - } + handleSetExpandedRow(ts) + // The logic to set disableAutoScrollRef.current = true on expansion + // is now handled by the useEffect hook that observes expandedRows. }, - [groupedMessages, expandedRows, scrollToBottomAuto, isAtBottom], + [handleSetExpandedRow], ) const handleRowHeightChange = useCallback( @@ -1111,6 +1141,20 @@ const ChatViewComponent: React.ForwardRefRenderFunction { + if (event?.shiftKey) { + // Always append to existing text, don't overwrite + setInputValue((currentValue) => { + return currentValue !== "" ? `${currentValue} \n${answer}` : answer + }) + } else { + handleSendMessage(answer, []) + } + }, + [handleSendMessage, setInputValue], // setInputValue is stable, handleSendMessage depends on clineAsk + ) + const itemContent = useCallback( (index: number, messageOrGroup: ClineMessage | ClineMessage[]) => { // browser session group @@ -1122,7 +1166,6 @@ const ChatViewComponent: React.ForwardRefRenderFunction expandedRows[messageTs] ?? false} onToggleExpand={(messageTs: number) => { setExpandedRows((prev) => ({ @@ -1140,32 +1183,25 @@ const ChatViewComponent: React.ForwardRefRenderFunction toggleRowExpansion(messageOrGroup.ts)} - lastModifiedMessage={modifiedMessages.at(-1)} - isLast={index === groupedMessages.length - 1} + onToggleExpand={toggleRowExpansion} // This was already stabilized + lastModifiedMessage={modifiedMessages.at(-1)} // Original direct access + isLast={index === groupedMessages.length - 1} // Original direct access onHeightChange={handleRowHeightChange} isStreaming={isStreaming} - onSuggestionClick={(answer: string, event?: React.MouseEvent) => { - if (event?.shiftKey) { - // Always append to existing text, don't overwrite - setInputValue((currentValue) => { - return currentValue !== "" ? `${currentValue} \n${answer}` : answer - }) - } else { - handleSendMessage(answer, []) - } - }} + onSuggestionClick={handleSuggestionClickInRow} // This was already stabilized /> ) }, [ + // Original broader dependencies expandedRows, + groupedMessages, modifiedMessages, - groupedMessages.length, handleRowHeightChange, isStreaming, toggleRowExpansion, - handleSendMessage, + handleSuggestionClickInRow, + setExpandedRows, // For the inline onToggleExpand in BrowserSessionRow ], ) From fe0681766c90820aaaf71ca1264e26cb4b1ce83d Mon Sep 17 00:00:00 2001 From: Daniel <57051444+daniel-lxs@users.noreply.github.com> Date: Wed, 28 May 2025 10:30:19 -0500 Subject: [PATCH 2/2] comment correct TTL --- webview-ui/src/components/chat/ChatView.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index 067d7b549a..5cd8c109ed 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -146,7 +146,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction>( new LRUCache({ max: 250, - ttl: 1000 * 60 * 15, // 1 hour TTL for long-running tasks + ttl: 1000 * 60 * 15, // 15 minutes TTL for long-running tasks }), )