-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Fix: Enhance prompt overwrites both edit and new message boxes #8554
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
base: main
Are you sure you want to change the base?
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 |
|---|---|---|
|
|
@@ -124,28 +124,33 @@ export const ChatTextArea = forwardRef<HTMLTextAreaElement, ChatTextAreaProps>( | |
| const message = event.data | ||
|
|
||
| if (message.type === "enhancedPrompt") { | ||
| if (message.text && textAreaRef.current) { | ||
| try { | ||
| // Use execCommand to replace text while preserving undo history | ||
| if (document.execCommand) { | ||
| // Use native browser methods to preserve undo stack | ||
| const textarea = textAreaRef.current | ||
|
|
||
| // Focus the textarea to ensure it's the active element | ||
| textarea.focus() | ||
|
|
||
| // Select all text first | ||
| textarea.select() | ||
| document.execCommand("insertText", false, message.text) | ||
| } else { | ||
| // Only handle the enhanced prompt if it's for this specific textarea instance | ||
| // Edit mode textareas have context "edit", main textarea has context "main" | ||
| const expectedContext = isEditMode ? "edit" : "main" | ||
| if (message.context === expectedContext) { | ||
| if (message.text && textAreaRef.current) { | ||
| try { | ||
| // Use execCommand to replace text while preserving undo history | ||
| if (document.execCommand) { | ||
| // Use native browser methods to preserve undo stack | ||
| const textarea = textAreaRef.current | ||
|
|
||
| // Focus the textarea to ensure it's the active element | ||
| textarea.focus() | ||
|
|
||
| // Select all text first | ||
| textarea.select() | ||
| document.execCommand("insertText", false, message.text) | ||
| } else { | ||
| setInputValue(message.text) | ||
| } | ||
| } catch { | ||
| setInputValue(message.text) | ||
| } | ||
| } catch { | ||
| setInputValue(message.text) | ||
| } | ||
| } | ||
|
|
||
| setIsEnhancingPrompt(false) | ||
| setIsEnhancingPrompt(false) | ||
| } | ||
| } else if (message.type === "insertTextIntoTextarea") { | ||
| if (message.text && textAreaRef.current) { | ||
| // Insert the command text at the current cursor position | ||
|
|
@@ -196,7 +201,7 @@ export const ChatTextArea = forwardRef<HTMLTextAreaElement, ChatTextAreaProps>( | |
|
|
||
| window.addEventListener("message", messageHandler) | ||
| return () => window.removeEventListener("message", messageHandler) | ||
| }, [setInputValue, searchRequestId, inputValue]) | ||
| }, [setInputValue, searchRequestId, inputValue, isEditMode]) | ||
|
Author
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. [P3] Performance: the message event listener re-subscribes on every inputValue change due to being in a useEffect with inputValue in deps. This can be avoided by using a ref for latest inputValue or the existing useEvent hook (already imported) so the handler stays stable while reading current state via refs, reducing listener churn. |
||
|
|
||
| const [isDraggingOver, setIsDraggingOver] = useState(false) | ||
| const [textAreaBaseHeight, setTextAreaBaseHeight] = useState<number | undefined>(undefined) | ||
|
|
@@ -239,11 +244,13 @@ export const ChatTextArea = forwardRef<HTMLTextAreaElement, ChatTextAreaProps>( | |
|
|
||
| if (trimmedInput) { | ||
| setIsEnhancingPrompt(true) | ||
| vscode.postMessage({ type: "enhancePrompt" as const, text: trimmedInput }) | ||
| // Include context to identify which textarea is requesting enhancement | ||
| const context = isEditMode ? "edit" : "main" | ||
| vscode.postMessage({ type: "enhancePrompt" as const, text: trimmedInput, context }) | ||
| } else { | ||
| setInputValue(t("chat:enhancePromptDescription")) | ||
| } | ||
| }, [inputValue, setInputValue, t]) | ||
| }, [inputValue, setInputValue, t, isEditMode]) | ||
|
|
||
| const allModes = useMemo(() => getAllModes(customModes), [customModes]) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,6 +112,7 @@ describe("ChatTextArea", () => { | |
| expect(mockPostMessage).toHaveBeenCalledWith({ | ||
| type: "enhancePrompt", | ||
| text: "Test prompt", | ||
| context: "main", | ||
| }) | ||
| }) | ||
|
|
||
|
|
@@ -183,7 +184,7 @@ describe("ChatTextArea", () => { | |
| }) | ||
|
|
||
| describe("enhanced prompt response", () => { | ||
|
Author
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. [P2] Missing error-path coverage: when an enhancedPrompt arrives with matching context but no text (error response), the component should still stop the loading spinner. Add a test asserting setIsEnhancingPrompt(false) is called (or the UI state reflects it) when message.text is undefined. |
||
| it("should update input value using native browser methods when receiving enhanced prompt", () => { | ||
| it("should update input value using native browser methods when receiving enhanced prompt with matching context", () => { | ||
| const setInputValue = vi.fn() | ||
|
|
||
| // Mock document.execCommand | ||
|
|
@@ -205,12 +206,13 @@ describe("ChatTextArea", () => { | |
| textarea.select = mockSelect | ||
| textarea.focus = mockFocus | ||
|
|
||
| // Simulate receiving enhanced prompt message | ||
| // Simulate receiving enhanced prompt message with matching context | ||
| window.dispatchEvent( | ||
| new MessageEvent("message", { | ||
| data: { | ||
| type: "enhancedPrompt", | ||
| text: "Enhanced test prompt", | ||
| context: "main", | ||
| }, | ||
| }), | ||
| ) | ||
|
|
@@ -221,6 +223,90 @@ describe("ChatTextArea", () => { | |
| expect(mockExecCommand).toHaveBeenCalledWith("insertText", false, "Enhanced test prompt") | ||
| }) | ||
|
|
||
| it("should ignore enhanced prompt with non-matching context", () => { | ||
| const setInputValue = vi.fn() | ||
|
|
||
| // Mock document.execCommand | ||
| const mockExecCommand = vi.fn().mockReturnValue(true) | ||
| Object.defineProperty(document, "execCommand", { | ||
| value: mockExecCommand, | ||
| writable: true, | ||
| }) | ||
|
|
||
| const { container } = render( | ||
| <ChatTextArea {...defaultProps} setInputValue={setInputValue} inputValue="Original prompt" />, | ||
| ) | ||
|
|
||
| const textarea = container.querySelector("textarea")! | ||
|
|
||
| // Mock textarea methods | ||
| const mockSelect = vi.fn() | ||
| const mockFocus = vi.fn() | ||
| textarea.select = mockSelect | ||
| textarea.focus = mockFocus | ||
|
|
||
| // Simulate receiving enhanced prompt message with non-matching context (edit instead of main) | ||
| window.dispatchEvent( | ||
| new MessageEvent("message", { | ||
| data: { | ||
| type: "enhancedPrompt", | ||
| text: "Enhanced test prompt", | ||
| context: "edit", | ||
| }, | ||
| }), | ||
| ) | ||
|
|
||
| // Verify native browser methods were NOT called | ||
| expect(mockFocus).not.toHaveBeenCalled() | ||
| expect(mockSelect).not.toHaveBeenCalled() | ||
| expect(mockExecCommand).not.toHaveBeenCalled() | ||
| expect(setInputValue).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("should handle enhanced prompt for edit mode", () => { | ||
| const setInputValue = vi.fn() | ||
|
|
||
| // Mock document.execCommand | ||
| const mockExecCommand = vi.fn().mockReturnValue(true) | ||
| Object.defineProperty(document, "execCommand", { | ||
| value: mockExecCommand, | ||
| writable: true, | ||
| }) | ||
|
|
||
| const { container } = render( | ||
| <ChatTextArea | ||
| {...defaultProps} | ||
| setInputValue={setInputValue} | ||
| inputValue="Original prompt" | ||
| isEditMode={true} | ||
| />, | ||
| ) | ||
|
|
||
| const textarea = container.querySelector("textarea")! | ||
|
|
||
| // Mock textarea methods | ||
| const mockSelect = vi.fn() | ||
| const mockFocus = vi.fn() | ||
| textarea.select = mockSelect | ||
| textarea.focus = mockFocus | ||
|
|
||
| // Simulate receiving enhanced prompt message with edit context | ||
| window.dispatchEvent( | ||
| new MessageEvent("message", { | ||
| data: { | ||
| type: "enhancedPrompt", | ||
| text: "Enhanced test prompt", | ||
| context: "edit", | ||
| }, | ||
| }), | ||
| ) | ||
|
|
||
| // Verify native browser methods were called for edit mode | ||
| expect(mockFocus).toHaveBeenCalled() | ||
| expect(mockSelect).toHaveBeenCalled() | ||
| expect(mockExecCommand).toHaveBeenCalledWith("insertText", false, "Enhanced test prompt") | ||
| }) | ||
|
|
||
| it("should fallback to setInputValue when execCommand is not available", () => { | ||
| const setInputValue = vi.fn() | ||
|
|
||
|
|
@@ -238,6 +324,7 @@ describe("ChatTextArea", () => { | |
| data: { | ||
| type: "enhancedPrompt", | ||
| text: "Enhanced test prompt", | ||
| context: "main", | ||
| }, | ||
| }), | ||
| ) | ||
|
|
@@ -258,6 +345,7 @@ describe("ChatTextArea", () => { | |
| data: { | ||
| type: "enhancedPrompt", | ||
| text: "Enhanced test prompt", | ||
| context: "main", | ||
| }, | ||
| }), | ||
| ) | ||
|
|
||
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.
[P2] Backward-compatibility edge case: if older backends emit 'enhancedPrompt' without a context field, this instance will ignore the event and the spinner may never clear. Consider defaulting undefined context to 'main' so the main textarea still handles legacy messages.
Example: