Chat focus issues #4072
Chat focus issues #4072
Conversation
dosubot
bot
added
size:L
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
moved this from Issue [In Progress]
to PR [Needs Preliminary Review]
in Roo Code Roadmap
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
daniel-lxs
left a comment
daniel-lxs
left a comment
There was a problem hiding this comment.
Thanks @xyOz-dev,
This looks like a good idea, I just left a couple of small points to improve the timeout logic.
| if (this.view?.visible) { | ||
| this.postMessageToWebview({ type: "action", action: "focusInput" }) | ||
| } | ||
| this.focusTimeoutId = undefined |
There was a problem hiding this comment.
I'm a bit concerned about potential race conditions here. You're using a 100ms timeout in the provider while ChatView uses 50ms. If someone rapidly switches between windows, multiple focus events could queue up and cause unexpected behavior.
Maybe we could use the same timeout duration in both places? Or even better, implement a proper debounce mechanism to handle rapid focus changes more gracefully.
| await this.removeClineFromStack() | ||
| this.log("Cleared task") | ||
|
|
||
| if (this.focusTimeoutId) { |
There was a problem hiding this comment.
Good that you're clearing the timeout in dispose(), but what happens if the view gets disposed while a timeout is still pending? The timeout would still fire and try to access this.view which might be disposed.
It might be a good idea to add a check in the timeout callback to ensure the view still exists and hasn't been disposed before trying to post a message to it.
| clearTimeout(focusTimeoutRef.current) | ||
| } | ||
|
|
||
| focusTimeoutRef.current = setTimeout(() => { |
There was a problem hiding this comment.
Since there's timeout logic here as well similar to what's on ClineProvider. Have you considered extracting this into a shared utility function or hook? Something like useDebouncedFocus() could handle the timeout.
| await provider.resolveWebviewView(mockWebviewView) | ||
|
|
||
| mockPostMessage.mockClear() | ||
|
|
There was a problem hiding this comment.
Would it make sense to swap out those arbitrary timeouts like 150ms and 10ms for Jest's timer mocks, like jest.useFakeTimers() and jest.runAllTimers()? Should make the test faster.
Also, we could test for rapid focus changes to ensure the debouncing is holding up well.
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Draft / In Progress]
in Roo Code Roadmap
* migrate apiConfiguration * fix type issue --------- Co-authored-by: Elephant Lumps <[email protected]>
|
stale |
github-project-automation
bot
moved this from PR [Draft / In Progress]
to Done
in Roo Code Roadmap
3 participants
Related GitHub Issue
Closes: #2189
Description
Fixes the related issue
Test Procedure
Type of Change
Side note its the first time an upstream change has come up in my commits, not sure if it should be removed or not but it was required to merge into current branch at time of PR Creation
Important
Fixes chat focus issues by adding timeout-based focus handling in
ClineProviderandChatView.tsx.ClineProviderto ensure input focus when window regains focus.ChatView.tsxto handlefocusInputaction with a timeout to ensure input focus.ClineProvider.test.tsto verify focus handling when window state changes.focusInputaction is tested for visibility and focus conditions.focusTimeoutIdinClineProviderandfocusTimeoutRefinChatView.tsxto manage focus timing.This description was created by
for fc12099. You can customize this summary. It will automatically update as commits are pushed.