Skip to content

Conversation

@kiwina
Copy link
Contributor

@kiwina kiwina commented Jun 2, 2025

PR Type

  • Bugfix

Description

This PR addresses multiple potential memory leaks in the ChatView component (webview-ui/src/components/chat/ChatView.tsx) related to unmanaged asynchronous operations and setTimeout calls.

1. Auto-Scroll Timeout Leak (ChatView_1113):

  • A useEffect hook (original line 1111) that schedules a setTimeout for scrollToBottomSmooth() did not clear the timeout.
  • Fix: The timeout ID is now captured and cleared in the useEffect's cleanup function.

2. Auto-Approve Async Delay Leak (ChatView_1236):

  • The autoApprove function (called by useEffect at original line 939) uses an await new Promise(setTimeout) for writeDelayMs. Subsequent state updates could occur on an unmounted component.
  • Fix: A isMountedRef is added to ChatViewComponent and checked before state updates within autoApprove after the delay. State updates are now grouped under a single isMountedRef.current check.

These changes ensure that asynchronous operations and timeouts are properly managed, preventing state updates on unmounted components and resolving potential memory leaks.

Related Issue

#4247

Changes

  • Modified webview-ui/src/components/chat/ChatView.tsx:
    • Added cleanup for the auto-scroll setTimeout.
    • Added isMountedRef and checks before state updates in the autoApprove function.

Testing

For Auto-Scroll Leak (ChatView_1113):

  1. Ensure disableAutoScrollRef.current is false.
  2. Trigger rapid changes in groupedMessages.length or quickly unmount ChatView (e.g., close the webview).
  3. Observe the console for "Can't perform a React state update on an unmounted component" warnings related to scrollToBottomSmooth. With the fix, this should not occur.

For Auto-Approve Leak (ChatView_1236):

  1. Configure an operation that triggers autoApprove with a writeDelayMs > 0 (e.g., an auto-approved file write operation).
  2. Trigger the operation.
  3. Quickly unmount the ChatView component before writeDelayMs elapses.
  4. Observe the console for warnings related to setSendingDisabled, setClineAsk, or setEnableButtons on an unmounted component. With the fix, these should not occur.

Important

Fixes memory leaks in ChatView.tsx by managing asynchronous operations and timeouts, ensuring no state updates on unmounted components.

  • Behavior:
    • Fixes memory leaks in ChatView.tsx by managing asynchronous operations and setTimeout calls.
    • Auto-scroll setTimeout now cleared in useEffect cleanup.
    • autoApprove function checks isMountedRef before state updates after delay.
  • State Management:
    • Adds isMountedRef to track component mount status.
    • Groups state updates under isMountedRef.current check in autoApprove.
  • Testing:
    • Auto-scroll: Ensure no warnings on unmounted component during rapid groupedMessages changes.
    • Auto-approve: Ensure no warnings on unmounted component when writeDelayMs elapses.

This description was created by Ellipsis for 480bf87. You can customize this summary. It will automatically update as commits are pushed.

@kiwina kiwina requested review from cte and mrubens as code owners June 2, 2025 08:21
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Jun 2, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jun 2, 2025
@daniel-lxs
Copy link
Member

Looks good to me, I didn't see any issues when testing however @cte left this comment on the commented out clearTimeout():

	useEffect(() => {
		if (!disableAutoScrollRef.current) {
			setTimeout(() => scrollToBottomSmooth(), 50)
			// Don't cleanup since if visibleMessages.length changes it cancels.
			// return () => clearTimeout(timer)
		}
	}, [groupedMessages.length, scrollToBottomSmooth])

So it might be a good idea that he takes a look.

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jun 2, 2025
@kiwina
Copy link
Contributor Author

kiwina commented Jun 2, 2025

Looks good to me, I didn't see any issues when testing however @cte left this comment on the commented out clearTimeout():

	useEffect(() => {
		if (!disableAutoScrollRef.current) {
			setTimeout(() => scrollToBottomSmooth(), 50)
			// Don't cleanup since if visibleMessages.length changes it cancels.
			// return () => clearTimeout(timer)
		}
	}, [groupedMessages.length, scrollToBottomSmooth])

So it might be a good idea that he takes a look.

Its never good to assume something gets done, better to implicitly take care of it than you know it is actually done

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 4, 2025
@mrubens
Copy link
Collaborator

mrubens commented Jun 4, 2025

Looks good to me if it looks good to @cte - he has more context than me.

@mrubens mrubens merged commit 3f3825a into RooCodeInc:main Jun 4, 2025
20 checks passed
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Jun 4, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jun 4, 2025
@hannesrudolph
Copy link
Collaborator

hi @kiwina

I work at Roo Code. Would you be able to reach out to me on discord? My username is hrudolph

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer PR - Needs Review size:S This PR changes 10-29 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants