-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Chat focus issues #4072
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chat focus issues #4072
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,6 +87,7 @@ export class ClineProvider | |
| return this._workspaceTracker | ||
| } | ||
| protected mcpHub?: McpHub // Change from private to protected | ||
| private focusTimeoutId: NodeJS.Timeout | undefined | ||
|
|
||
| public isViewLaunched = false | ||
| public settingsImportedAt?: number | ||
|
|
@@ -220,6 +221,11 @@ export class ClineProvider | |
| await this.removeClineFromStack() | ||
| this.log("Cleared task") | ||
|
|
||
| if (this.focusTimeoutId) { | ||
| clearTimeout(this.focusTimeoutId) | ||
| this.focusTimeoutId = undefined | ||
| } | ||
|
|
||
| if (this.view && "dispose" in this.view) { | ||
| this.view.dispose() | ||
| this.log("Disposed webview") | ||
|
|
@@ -447,6 +453,25 @@ export class ClineProvider | |
| this.disposables, | ||
| ) | ||
|
|
||
| vscode.window.onDidChangeWindowState?.( | ||
| (windowState) => { | ||
| if (windowState.focused && this.view?.visible) { | ||
| if (this.focusTimeoutId) { | ||
| clearTimeout(this.focusTimeoutId) | ||
| } | ||
|
|
||
| this.focusTimeoutId = setTimeout(() => { | ||
| if (this.view?.visible) { | ||
| this.postMessageToWebview({ type: "action", action: "focusInput" }) | ||
| } | ||
| this.focusTimeoutId = undefined | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| }, 100) | ||
| } | ||
| }, | ||
| null, | ||
| this.disposables, | ||
| ) | ||
|
|
||
| // Listen for when color changes | ||
| vscode.workspace.onDidChangeConfiguration( | ||
| async (e) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -150,6 +150,9 @@ jest.mock("vscode", () => ({ | |
| window: { | ||
| showInformationMessage: jest.fn(), | ||
| showErrorMessage: jest.fn(), | ||
| onDidChangeWindowState: jest.fn().mockImplementation(() => ({ | ||
| dispose: jest.fn(), | ||
| })), | ||
| }, | ||
| workspace: { | ||
| getConfiguration: jest.fn().mockReturnValue({ | ||
|
|
@@ -282,14 +285,12 @@ describe("ClineProvider", () => { | |
| dispose: jest.fn(), | ||
| } | ||
|
|
||
| // Mock output channel | ||
| mockOutputChannel = { | ||
| appendLine: jest.fn(), | ||
| clear: jest.fn(), | ||
| dispose: jest.fn(), | ||
| } as unknown as vscode.OutputChannel | ||
|
|
||
| // Mock webview | ||
| mockPostMessage = jest.fn() | ||
|
|
||
| mockWebviewView = { | ||
|
|
@@ -2397,4 +2398,168 @@ describe("ClineProvider - Router Models", () => { | |
| }, | ||
| }) | ||
| }) | ||
|
|
||
| describe("Window Focus Handling", () => { | ||
| let provider: ClineProvider | ||
| let mockContext: vscode.ExtensionContext | ||
| let mockOutputChannel: vscode.OutputChannel | ||
| let mockWebviewView: vscode.WebviewView | ||
| let mockPostMessage: jest.Mock | ||
| let mockWindowStateListeners: Array<(e: vscode.WindowState) => void> = [] | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks() | ||
|
|
||
| mockWindowStateListeners = [] | ||
|
|
||
| ;(vscode.window.onDidChangeWindowState as jest.Mock).mockImplementation((listener) => { | ||
| mockWindowStateListeners.push(listener) | ||
| return { dispose: jest.fn() } | ||
| }) | ||
|
|
||
| const globalState: Record<string, string | undefined> = { | ||
| mode: "code", | ||
| } | ||
|
|
||
| const secrets: Record<string, string | undefined> = {} | ||
|
|
||
| mockContext = { | ||
| extensionPath: "/test/path", | ||
| extensionUri: {} as vscode.Uri, | ||
| globalState: { | ||
| get: jest.fn().mockImplementation((key: string) => globalState[key]), | ||
| update: jest.fn().mockImplementation((key: string, value: string | undefined) => (globalState[key] = value)), | ||
| keys: jest.fn().mockImplementation(() => Object.keys(globalState)), | ||
| }, | ||
| secrets: { | ||
| get: jest.fn().mockImplementation((key: string) => secrets[key]), | ||
| store: jest.fn().mockImplementation((key: string, value: string | undefined) => (secrets[key] = value)), | ||
| delete: jest.fn().mockImplementation((key: string) => delete secrets[key]), | ||
| }, | ||
| subscriptions: [], | ||
| extension: { | ||
| packageJSON: { version: "1.0.0" }, | ||
| }, | ||
| globalStorageUri: { | ||
| fsPath: "/test/storage/path", | ||
| }, | ||
| } as unknown as vscode.ExtensionContext | ||
|
|
||
| // Mock output channel | ||
| mockOutputChannel = { | ||
| appendLine: jest.fn(), | ||
| clear: jest.fn(), | ||
| dispose: jest.fn(), | ||
| } as unknown as vscode.OutputChannel | ||
|
|
||
| // Mock webview | ||
| mockPostMessage = jest.fn() | ||
|
|
||
| mockWebviewView = { | ||
| webview: { | ||
| postMessage: mockPostMessage, | ||
| html: "", | ||
| options: {}, | ||
| onDidReceiveMessage: jest.fn(), | ||
| asWebviewUri: jest.fn(), | ||
| }, | ||
| visible: true, | ||
| onDidDispose: jest.fn().mockImplementation((callback) => { | ||
| callback() | ||
| return { dispose: jest.fn() } | ||
| }), | ||
| onDidChangeVisibility: jest.fn().mockImplementation(() => ({ dispose: jest.fn() })), | ||
| } as unknown as vscode.WebviewView | ||
|
|
||
| provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", new ContextProxy(mockContext)) | ||
| }) | ||
|
|
||
| test("registers window state change listener on webview resolution", async () => { | ||
| await provider.resolveWebviewView(mockWebviewView) | ||
|
|
||
| expect(vscode.window.onDidChangeWindowState).toHaveBeenCalled() | ||
| expect(mockWindowStateListeners.length).toBeGreaterThan(0) | ||
| }) | ||
|
|
||
| test("sends focusInput message when window becomes focused and webview is visible", async () => { | ||
| await provider.resolveWebviewView(mockWebviewView) | ||
|
|
||
| mockPostMessage.mockClear() | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| const windowState: vscode.WindowState = { focused: true, active: true } | ||
| mockWindowStateListeners.forEach((listener) => listener(windowState)) | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 150)) | ||
|
|
||
| expect(mockPostMessage).toHaveBeenCalledWith({ | ||
| type: "action", | ||
| action: "focusInput", | ||
| }) | ||
| }) | ||
|
|
||
| test("does not send focusInput message when window becomes focused but webview is not visible", async () => { | ||
| const invisibleWebviewView = { | ||
| ...mockWebviewView, | ||
| visible: false, | ||
| } | ||
| await provider.resolveWebviewView(invisibleWebviewView) | ||
|
|
||
| mockPostMessage.mockClear() | ||
|
|
||
| const windowState: vscode.WindowState = { focused: true, active: true } | ||
| mockWindowStateListeners.forEach((listener) => listener(windowState)) | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 150)) | ||
|
|
||
| expect(mockPostMessage).not.toHaveBeenCalledWith({ | ||
| type: "action", | ||
| action: "focusInput", | ||
| }) | ||
| }) | ||
|
|
||
| test("does not send focusInput message when window loses focus", async () => { | ||
| await provider.resolveWebviewView(mockWebviewView) | ||
|
|
||
| mockPostMessage.mockClear() | ||
|
|
||
| const windowState: vscode.WindowState = { focused: false, active: false } | ||
| mockWindowStateListeners.forEach((listener) => listener(windowState)) | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 150)) | ||
|
|
||
| expect(mockPostMessage).not.toHaveBeenCalledWith({ | ||
| type: "action", | ||
| action: "focusInput", | ||
| }) | ||
| }) | ||
|
|
||
| test("properly disposes window state listener and clears timeout", async () => { | ||
| await provider.resolveWebviewView(mockWebviewView) | ||
|
|
||
| expect(vscode.window.onDidChangeWindowState).toHaveBeenCalled() | ||
| expect(mockWindowStateListeners.length).toBeGreaterThan(0) | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, 10)) | ||
|
|
||
| const windowState: vscode.WindowState = { focused: true, active: true } | ||
| mockWindowStateListeners.forEach((listener) => listener(windowState)) | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, 10)) | ||
|
|
||
| const timeoutId = (provider as any).focusTimeoutId | ||
| expect(timeoutId).toBeDefined() | ||
|
|
||
| const clearTimeoutSpy = jest.spyOn(global, 'clearTimeout') | ||
|
|
||
| await provider.dispose() | ||
|
|
||
| expect(clearTimeoutSpy).toHaveBeenCalledWith(timeoutId) | ||
|
|
||
| expect((provider as any).focusTimeoutId).toBeUndefined() | ||
|
|
||
| expect((provider as any).disposables).toEqual([]) | ||
|
|
||
| clearTimeoutSpy.mockRestore() | ||
| }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,6 +135,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |
| const lastTtsRef = useRef<string>("") | ||
| const [wasStreaming, setWasStreaming] = useState<boolean>(false) | ||
| const [showCheckpointWarning, setShowCheckpointWarning] = useState<boolean>(false) | ||
| const focusTimeoutRef = useRef<NodeJS.Timeout | null>(null) | ||
| const [isCondensing, setIsCondensing] = useState<boolean>(false) | ||
|
|
||
| // UI layout depends on the last 2 messages | ||
|
|
@@ -587,7 +588,20 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |
| } | ||
| break | ||
| case "focusInput": | ||
| textAreaRef.current?.focus() | ||
| if (focusTimeoutRef.current) { | ||
| clearTimeout(focusTimeoutRef.current) | ||
| } | ||
|
|
||
| focusTimeoutRef.current = setTimeout(() => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| if (!isHidden && !sendingDisabled && !enableButtons && textAreaRef.current) { | ||
| try { | ||
| textAreaRef.current.focus() | ||
| } catch (e) { | ||
| console.debug("Failed to focus input:", e) | ||
| } | ||
| } | ||
| focusTimeoutRef.current = null | ||
| }, 50) | ||
| break | ||
| } | ||
| break | ||
|
|
@@ -650,6 +664,15 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |
| // NOTE: the VSCode window needs to be focused for this to work. | ||
| useMount(() => textAreaRef.current?.focus()) | ||
|
|
||
| // Cleanup effect for focus timeout | ||
| useEffect(() => { | ||
| return () => { | ||
| if (focusTimeoutRef.current) { | ||
| clearTimeout(focusTimeoutRef.current) | ||
| } | ||
| } | ||
| }, []) | ||
|
|
||
| useEffect(() => { | ||
| const timer = setTimeout(() => { | ||
| if (!isHidden && !sendingDisabled && !enableButtons) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.