-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: ensure user messages are sent with save button for file editing operations #6480
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
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 |
|---|---|---|
|
|
@@ -723,19 +723,42 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |
| case "use_mcp_server": | ||
| case "resume_task": | ||
| case "mistake_limit_reached": | ||
| // Only send text/images if they exist | ||
| if (trimmedInput || (images && images.length > 0)) { | ||
| vscode.postMessage({ | ||
| type: "askResponse", | ||
| askResponse: "yesButtonClicked", | ||
| text: trimmedInput, | ||
| images: images, | ||
| }) | ||
| // For tool operations (like file editing), check if we have current input or queued messages | ||
| if (clineAsk === "tool") { | ||
| // Get current input from the text area (this includes any text typed while agent was working) | ||
| const currentInput = text?.trim() || inputValue.trim() | ||
|
Contributor
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. Could we use more descriptive variable names here? Something like and would make it clearer that these are specifically for tool operations. |
||
| const currentImages = images || selectedImages | ||
|
|
||
| // Send the save action with any current input | ||
| if (currentInput || (currentImages && currentImages.length > 0)) { | ||
| vscode.postMessage({ | ||
| type: "askResponse", | ||
| askResponse: "yesButtonClicked", | ||
| text: currentInput, | ||
| images: currentImages, | ||
| }) | ||
| } else { | ||
| vscode.postMessage({ type: "askResponse", askResponse: "yesButtonClicked" }) | ||
| } | ||
|
|
||
| // Clear input state after sending | ||
| setInputValue("") | ||
| setSelectedImages([]) | ||
| } else { | ||
| vscode.postMessage({ type: "askResponse", askResponse: "yesButtonClicked" }) | ||
| // For other operations, use the original logic | ||
| if (trimmedInput || (images && images.length > 0)) { | ||
| vscode.postMessage({ | ||
| type: "askResponse", | ||
| askResponse: "yesButtonClicked", | ||
| text: trimmedInput, | ||
| images: images, | ||
| }) | ||
| // Clear input state after sending | ||
| setInputValue("") | ||
| setSelectedImages([]) | ||
| } else { | ||
| vscode.postMessage({ type: "askResponse", askResponse: "yesButtonClicked" }) | ||
| } | ||
| } | ||
| break | ||
| case "completion_result": | ||
|
|
@@ -752,7 +775,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |
| setClineAsk(undefined) | ||
| setEnableButtons(false) | ||
| }, | ||
| [clineAsk, startNewTask], | ||
| [clineAsk, startNewTask, inputValue, selectedImages, setInputValue, setSelectedImages], | ||
|
Contributor
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. The dependency array includes and which are setState functions and should be stable. Could we verify this doesn't cause unnecessary re-renders? These might be safely omitted from the dependency array. |
||
| ) | ||
|
|
||
| const handleSecondaryButtonClick = useCallback( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,170 @@ | ||||||
| import React from "react" | ||||||
| import { render, screen, fireEvent } from "@testing-library/react" | ||||||
| import { vi } from "vitest" | ||||||
| import QueuedMessages from "../QueuedMessages" | ||||||
| import { QueuedMessage } from "@roo-code/types" | ||||||
|
|
||||||
| // Mock react-i18next | ||||||
| vi.mock("react-i18next", () => ({ | ||||||
| useTranslation: () => ({ | ||||||
| t: (key: string) => key, | ||||||
| }), | ||||||
| })) | ||||||
|
|
||||||
| // Mock the Mention component | ||||||
| vi.mock("../Mention", () => ({ | ||||||
| Mention: ({ text }: { text: string }) => <span data-testid="mention">{text}</span>, | ||||||
| })) | ||||||
|
|
||||||
| // Mock the Thumbnails component | ||||||
| vi.mock("../common/Thumbnails", () => ({ | ||||||
| default: ({ images }: { images: string[] }) => <div data-testid="thumbnails">{images.length} images</div>, | ||||||
|
Contributor
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. The mock for the Thumbnails component returns a with a data-testid rather than actual
Suggested change
|
||||||
| })) | ||||||
|
|
||||||
| // Mock the Button component | ||||||
| vi.mock("@src/components/ui", () => ({ | ||||||
| Button: ({ children, onClick, ...props }: any) => ( | ||||||
| <button onClick={onClick} {...props}> | ||||||
| {children} | ||||||
| </button> | ||||||
| ), | ||||||
| })) | ||||||
|
|
||||||
| describe("QueuedMessages", () => { | ||||||
| const mockOnRemove = vi.fn() | ||||||
| const mockOnUpdate = vi.fn() | ||||||
|
|
||||||
| beforeEach(() => { | ||||||
| vi.clearAllMocks() | ||||||
| }) | ||||||
|
|
||||||
| it("renders nothing when queue is empty", () => { | ||||||
| const { container } = render(<QueuedMessages queue={[]} onRemove={mockOnRemove} onUpdate={mockOnUpdate} />) | ||||||
| expect(container.firstChild).toBeNull() | ||||||
| }) | ||||||
|
|
||||||
| it("renders queued messages", () => { | ||||||
| const queue: QueuedMessage[] = [ | ||||||
| { | ||||||
| id: "1", | ||||||
| text: "Test message 1", | ||||||
| images: [], | ||||||
| }, | ||||||
| { | ||||||
| id: "2", | ||||||
| text: "Test message 2", | ||||||
| images: ["image1.png"], | ||||||
| }, | ||||||
| ] | ||||||
|
|
||||||
| render(<QueuedMessages queue={queue} onRemove={mockOnRemove} onUpdate={mockOnUpdate} />) | ||||||
|
|
||||||
| expect(screen.getByTestId("queued-messages")).toBeInTheDocument() | ||||||
| expect(screen.getByText("queuedMessages.title")).toBeInTheDocument() | ||||||
| expect(screen.getAllByTestId("mention")).toHaveLength(2) | ||||||
| }) | ||||||
|
|
||||||
| it("calls onRemove when delete button is clicked", () => { | ||||||
| const queue: QueuedMessage[] = [ | ||||||
| { | ||||||
| id: "1", | ||||||
| text: "Test message", | ||||||
| images: [], | ||||||
| }, | ||||||
| ] | ||||||
|
|
||||||
| render(<QueuedMessages queue={queue} onRemove={mockOnRemove} onUpdate={mockOnUpdate} />) | ||||||
|
|
||||||
| const deleteButton = screen.getByRole("button") | ||||||
| fireEvent.click(deleteButton) | ||||||
|
|
||||||
| expect(mockOnRemove).toHaveBeenCalledWith(0) | ||||||
| }) | ||||||
|
|
||||||
| it("enters edit mode when message is clicked", () => { | ||||||
| const queue: QueuedMessage[] = [ | ||||||
| { | ||||||
| id: "1", | ||||||
| text: "Test message", | ||||||
| images: [], | ||||||
| }, | ||||||
| ] | ||||||
|
|
||||||
| render(<QueuedMessages queue={queue} onRemove={mockOnRemove} onUpdate={mockOnUpdate} />) | ||||||
|
|
||||||
| const messageElement = screen.getByTestId("mention").parentElement | ||||||
| fireEvent.click(messageElement!) | ||||||
|
|
||||||
| expect(screen.getByRole("textbox")).toBeInTheDocument() | ||||||
| }) | ||||||
|
|
||||||
| it("calls onUpdate when edit is saved", () => { | ||||||
| const queue: QueuedMessage[] = [ | ||||||
| { | ||||||
| id: "1", | ||||||
| text: "Test message", | ||||||
| images: [], | ||||||
| }, | ||||||
| ] | ||||||
|
|
||||||
| render(<QueuedMessages queue={queue} onRemove={mockOnRemove} onUpdate={mockOnUpdate} />) | ||||||
|
|
||||||
| // Enter edit mode | ||||||
| const messageElement = screen.getByTestId("mention").parentElement | ||||||
| fireEvent.click(messageElement!) | ||||||
|
|
||||||
| // Edit the text | ||||||
| const textarea = screen.getByRole("textbox") | ||||||
| fireEvent.change(textarea, { target: { value: "Updated message" } }) | ||||||
|
|
||||||
| // Save by pressing Enter | ||||||
| fireEvent.keyDown(textarea, { key: "Enter" }) | ||||||
|
|
||||||
| expect(mockOnUpdate).toHaveBeenCalledWith(0, "Updated message") | ||||||
| }) | ||||||
|
|
||||||
| it("cancels edit when Escape is pressed", () => { | ||||||
| const queue: QueuedMessage[] = [ | ||||||
| { | ||||||
| id: "1", | ||||||
| text: "Test message", | ||||||
| images: [], | ||||||
| }, | ||||||
| ] | ||||||
|
|
||||||
| render(<QueuedMessages queue={queue} onRemove={mockOnRemove} onUpdate={mockOnUpdate} />) | ||||||
|
|
||||||
| // Enter edit mode | ||||||
| const messageElement = screen.getByTestId("mention").parentElement | ||||||
| fireEvent.click(messageElement!) | ||||||
|
|
||||||
| // Edit the text | ||||||
| const textarea = screen.getByRole("textbox") | ||||||
| fireEvent.change(textarea, { target: { value: "Updated message" } }) | ||||||
|
|
||||||
| // Cancel by pressing Escape | ||||||
| fireEvent.keyDown(textarea, { key: "Escape" }) | ||||||
|
|
||||||
| // Should not call onUpdate and should exit edit mode | ||||||
| expect(mockOnUpdate).not.toHaveBeenCalled() | ||||||
| expect(screen.queryByRole("textbox")).not.toBeInTheDocument() | ||||||
| }) | ||||||
|
|
||||||
| it("renders thumbnails for messages with images", () => { | ||||||
| const queue: QueuedMessage[] = [ | ||||||
| { | ||||||
| id: "1", | ||||||
| text: "Message with images", | ||||||
| images: ["image1.png", "image2.png"], | ||||||
| }, | ||||||
| ] | ||||||
|
|
||||||
| render(<QueuedMessages queue={queue} onRemove={mockOnRemove} onUpdate={mockOnUpdate} />) | ||||||
|
|
||||||
| // Check that images are rendered (the actual Thumbnails component renders img elements) | ||||||
| const images = screen.getAllByRole("img") | ||||||
|
Contributor
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. The test expects elements but the mocked Thumbnails component returns a div with text content. This creates a mismatch between what the test expects and what the mock provides. Could we update the mock to render actual img elements or adjust the test expectations? |
||||||
| expect(images).toHaveLength(2) | ||||||
| expect(images[0]).toHaveAttribute("src", "image1.png") | ||||||
| expect(images[1]).toHaveAttribute("src", "image2.png") | ||||||
| }) | ||||||
| }) | ||||||
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.
Is this intentional? The special case for duplicates much of the logic from the else branch (lines 748-762). Could we refactor this to reduce duplication while maintaining the specific behavior for tool operations?