-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add submitUserMessage to Task #6895
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
Conversation
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.
Thank you for your contribution! I've reviewed the changes and found this to be a well-implemented feature. The code is clean, properly tested, and follows established patterns. I have a few minor suggestions for improvement.
|
|
||
| public submitUserMessage(text: string, images?: string[]): void { | ||
| try { | ||
| const trimmed = (text ?? "").trim() |
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.
Consider simplifying the null handling here. You're using text ?? "" but then checking !trimmed. This could be simplified:
| const trimmed = (text ?? "").trim() | |
| const trimmed = text?.trim() || "" | |
| const imgs = images || [] | |
| if (!trimmed && imgs.length === 0) { | |
| return | |
| } |
This would be more consistent with how you handle the images parameter.
| this.askResponseImages = images | ||
| } | ||
|
|
||
| public submitUserMessage(text: string, images?: string[]): void { |
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.
Consider adding JSDoc documentation for this public method to maintain consistency with other public methods and improve IDE tooltips:
| public submitUserMessage(text: string, images?: string[]): void { | |
| /** | |
| * Submits a user message to the webview. | |
| * @param text - The message text to submit | |
| * @param images - Optional array of image paths to include with the message | |
| */ | |
| public submitUserMessage(text: string, images?: string[]): void { |
| expect(noModelTask.apiConfiguration.apiProvider).toBe("openai") | ||
| }) | ||
| }) | ||
|
|
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 test coverage! Consider adding one more test case for completeness - testing the scenario where only images are provided without text:
it("should handle images-only messages", async () => {
const task = new Task({
provider: mockProvider,
apiConfiguration: mockApiConfig,
task: "initial task",
startTask: false,
})
// Call with images but no text
task.submitUserMessage("", ["image1.png", "image2.png"])
// Should not call postMessageToWebview since text is empty
expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled()
})This would ensure the edge case is explicitly tested, even though the current implementation handles it correctly.
Important
Adds
submitUserMessagemethod toTaskclass for submitting user messages to a webview, with tests for various scenarios.submitUserMessagemethod toTaskclass inTask.tsto submit user messages to a webview.submitUserMessagetoTaskLikeinterface intask.ts.Task.spec.tsto verifysubmitUserMessagebehavior, including handling of empty messages and undefined providers.package.jsonfrom1.44.0to1.45.0.This description was created by
for f877d30. You can customize this summary. It will automatically update as commits are pushed.