-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: allow users to change mode when approving orchestrator subtasks #6708
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 |
|---|---|---|
|
|
@@ -225,8 +225,14 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| private askResponse?: ClineAskResponse | ||
| private askResponseText?: string | ||
| private askResponseImages?: string[] | ||
| private askResponseValues?: Record<string, any> | ||
| public lastMessageTs?: number | ||
|
|
||
| // Getter for askResponseValues to allow tools to access it | ||
| get getAskResponseValues(): Record<string, any> | undefined { | ||
|
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. Nice addition of the getter! Consider adding a JSDoc comment to document what this getter returns and when it's used. This would help future maintainers understand the askResponseValues flow better. |
||
| return this.askResponseValues | ||
| } | ||
|
|
||
| // Tool Use | ||
| consecutiveMistakeCount: number = 0 | ||
| consecutiveMistakeLimit: number | ||
|
|
@@ -742,10 +748,19 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| this.handleWebviewAskResponse("messageResponse", text, images) | ||
| } | ||
|
|
||
| handleWebviewAskResponse(askResponse: ClineAskResponse, text?: string, images?: string[]) { | ||
| handleWebviewAskResponse( | ||
| askResponse: ClineAskResponse, | ||
| text?: string, | ||
| images?: string[], | ||
| values?: Record<string, any>, | ||
| ) { | ||
| this.askResponse = askResponse | ||
| this.askResponseText = text | ||
| this.askResponseImages = images | ||
| // Store values for later use if needed | ||
| if (values) { | ||
| this.askResponseValues = values | ||
| } | ||
| } | ||
|
|
||
| async handleTerminalOperation(terminalOperation: "continue" | "abort") { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ const mockCline = { | |
| consecutiveMistakeCount: 0, | ||
| isPaused: false, | ||
| pausedModeSlug: "ask", | ||
| getAskResponseValues: undefined as Record<string, any> | undefined, | ||
| providerRef: { | ||
| deref: vi.fn(() => ({ | ||
| getState: vi.fn(() => ({ customModes: [], mode: "ask" })), | ||
|
|
@@ -184,4 +185,143 @@ describe("newTaskTool", () => { | |
| }) | ||
|
|
||
| // Add more tests for error handling (missing params, invalid mode, approval denied) if needed | ||
|
|
||
| it("should use user-selected mode when provided in askResponseValues", async () => { | ||
|
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. Great test coverage! The edge cases are well handled. Consider adding an integration test that verifies the full flow from UI interaction to task creation with the selected mode - this would give us confidence that all the pieces work together correctly.
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. Great test coverage! The edge cases are well handled. Consider adding an integration test that verifies the full flow from UI interaction to task creation with the selected mode - this would give us confidence that all the pieces work together correctly. |
||
| const block: ToolUse = { | ||
| type: "tool_use", | ||
| name: "new_task", | ||
| params: { | ||
| mode: "code", | ||
| message: "Create a new feature", | ||
| }, | ||
| partial: false, | ||
| } | ||
|
|
||
| // Mock user selecting a different mode | ||
| mockCline.getAskResponseValues = { selectedMode: "architect" } | ||
|
|
||
| // Mock the architect mode | ||
| vi.mocked(getModeBySlug).mockImplementation((slug) => { | ||
| if (slug === "architect") { | ||
| return { | ||
| slug: "architect", | ||
| name: "Architect Mode", | ||
| roleDefinition: "Architecture role definition", | ||
| groups: ["command", "read"], | ||
| } | ||
| } | ||
| return { | ||
| slug: "code", | ||
| name: "Code Mode", | ||
| roleDefinition: "Test role definition", | ||
| groups: ["command", "read", "edit"], | ||
| } | ||
| }) | ||
|
|
||
| const mockHandleModeSwitch = vi.fn() | ||
| mockCline.providerRef.deref = vi.fn(() => ({ | ||
| getState: vi.fn(() => ({ customModes: [], mode: "ask" })), | ||
| handleModeSwitch: mockHandleModeSwitch, | ||
| initClineWithTask: mockInitClineWithTask, | ||
| })) | ||
|
|
||
| await newTaskTool( | ||
| mockCline as any, | ||
| block, | ||
| mockAskApproval, | ||
| mockHandleError, | ||
| mockPushToolResult, | ||
| mockRemoveClosingTag, | ||
| ) | ||
|
|
||
| // Verify the mode switch was called with the user-selected mode | ||
| expect(mockHandleModeSwitch).toHaveBeenCalledWith("architect") | ||
|
|
||
| // Verify the success message includes the correct mode name | ||
| expect(mockPushToolResult).toHaveBeenCalledWith( | ||
| expect.stringContaining("Successfully created new task in Architect Mode"), | ||
| ) | ||
| }) | ||
|
|
||
| it("should use original mode when no user selection is provided", async () => { | ||
| const block: ToolUse = { | ||
| type: "tool_use", | ||
| name: "new_task", | ||
| params: { | ||
| mode: "code", | ||
| message: "Create a new feature", | ||
| }, | ||
| partial: false, | ||
| } | ||
|
|
||
| // No user selection | ||
| mockCline.getAskResponseValues = undefined | ||
|
|
||
| const mockHandleModeSwitch = vi.fn() | ||
| mockCline.providerRef.deref = vi.fn(() => ({ | ||
| getState: vi.fn(() => ({ customModes: [], mode: "ask" })), | ||
| handleModeSwitch: mockHandleModeSwitch, | ||
| initClineWithTask: mockInitClineWithTask, | ||
| })) | ||
|
|
||
| await newTaskTool( | ||
| mockCline as any, | ||
| block, | ||
| mockAskApproval, | ||
| mockHandleError, | ||
| mockPushToolResult, | ||
| mockRemoveClosingTag, | ||
| ) | ||
|
|
||
| // Verify the mode switch was called with the original mode | ||
| expect(mockHandleModeSwitch).toHaveBeenCalledWith("code") | ||
|
|
||
| // Verify the success message includes the correct mode name | ||
| expect(mockPushToolResult).toHaveBeenCalledWith( | ||
| expect.stringContaining("Successfully created new task in Code Mode"), | ||
| ) | ||
| }) | ||
|
|
||
| it("should handle invalid user-selected mode gracefully", async () => { | ||
| const block: ToolUse = { | ||
| type: "tool_use", | ||
| name: "new_task", | ||
| params: { | ||
| mode: "code", | ||
| message: "Create a new feature", | ||
| }, | ||
| partial: false, | ||
| } | ||
|
|
||
| // Mock user selecting an invalid mode | ||
| mockCline.getAskResponseValues = { selectedMode: "invalid-mode" } | ||
|
|
||
| // Mock getModeBySlug to return undefined for invalid mode | ||
| vi.mocked(getModeBySlug).mockImplementation((slug) => { | ||
| if (slug === "invalid-mode") { | ||
| return undefined | ||
| } | ||
| return { | ||
| slug: "code", | ||
| name: "Code Mode", | ||
| roleDefinition: "Test role definition", | ||
| groups: ["command", "read", "edit"], | ||
| } | ||
| }) | ||
|
|
||
| await newTaskTool( | ||
| mockCline as any, | ||
| block, | ||
| mockAskApproval, | ||
| mockHandleError, | ||
| mockPushToolResult, | ||
| mockRemoveClosingTag, | ||
| ) | ||
|
|
||
| // Verify error was pushed | ||
| expect(mockPushToolResult).toHaveBeenCalledWith("Tool Error: Invalid mode: invalid-mode") | ||
|
|
||
| // Verify no task was created | ||
| expect(mockInitClineWithTask).not.toHaveBeenCalled() | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ import MarkdownBlock from "../common/MarkdownBlock" | |
| import { ReasoningBlock } from "./ReasoningBlock" | ||
| import Thumbnails from "../common/Thumbnails" | ||
| import McpResourceRow from "../mcp/McpResourceRow" | ||
| import ModeSelector from "./ModeSelector" | ||
|
|
||
| import { Mention } from "./Mention" | ||
| import { CheckpointSaved } from "./checkpoints/CheckpointSaved" | ||
|
|
@@ -60,6 +61,7 @@ interface ChatRowProps { | |
| onFollowUpUnmount?: () => void | ||
| isFollowUpAnswered?: boolean | ||
| editable?: boolean | ||
| onNewTaskModeChange?: (mode: string) => void | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-empty-object-type | ||
|
|
@@ -112,16 +114,18 @@ export const ChatRowContent = ({ | |
| onBatchFileResponse, | ||
| isFollowUpAnswered, | ||
| editable, | ||
| onNewTaskModeChange, | ||
| }: ChatRowContentProps) => { | ||
| const { t } = useTranslation() | ||
| const { mcpServers, alwaysAllowMcp, currentCheckpoint, mode } = useExtensionState() | ||
| const { mcpServers, alwaysAllowMcp, currentCheckpoint, mode, customModes, customModePrompts } = useExtensionState() | ||
| const [reasoningCollapsed, setReasoningCollapsed] = useState(true) | ||
| const [isDiffErrorExpanded, setIsDiffErrorExpanded] = useState(false) | ||
| const [showCopySuccess, setShowCopySuccess] = useState(false) | ||
| const [isEditing, setIsEditing] = useState(false) | ||
| const [editedContent, setEditedContent] = useState("") | ||
| const [editMode, setEditMode] = useState<Mode>(mode || "code") | ||
| const [editImages, setEditImages] = useState<string[]>([]) | ||
| const [selectedNewTaskMode, setSelectedNewTaskMode] = useState<Mode | null>(null) | ||
| const { copyWithFeedback } = useCopyToClipboard() | ||
|
|
||
| // Handle message events for image selection during edit mode | ||
|
|
@@ -765,16 +769,39 @@ export const ChatRowContent = ({ | |
| </> | ||
| ) | ||
| case "newTask": | ||
| // Use the selected mode if available, otherwise use the tool's mode | ||
| const effectiveMode = selectedNewTaskMode || tool.mode | ||
|
|
||
| return ( | ||
| <> | ||
| <div style={headerStyle}> | ||
| {toolIcon("tasklist")} | ||
| <span style={{ fontWeight: "bold" }}> | ||
| <Trans | ||
| i18nKey="chat:subtasks.wantsToCreate" | ||
| components={{ code: <code>{tool.mode}</code> }} | ||
| values={{ mode: tool.mode }} | ||
| /> | ||
| <span style={{ fontWeight: "bold", display: "flex", alignItems: "center", gap: "4px" }}> | ||
| {t("chat:subtasks.wantsToCreate").split("{mode}")[0]} | ||
| {message.type === "ask" ? ( | ||
| <ModeSelector | ||
|
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 mode selector integration looks clean! A small UI enhancement to consider: when in "ask" state, the current mode could be shown more prominently to help users understand what they're changing from.
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 mode selector integration looks clean! A small UI enhancement to consider: when in "ask" state, the current mode could be shown more prominently to help users understand what they're changing from. |
||
| value={effectiveMode as Mode} | ||
| onChange={(newMode) => { | ||
| setSelectedNewTaskMode(newMode) | ||
| // Update the tool data with the new mode | ||
| if (tool) { | ||
| tool.mode = newMode | ||
| } | ||
| // Notify parent component | ||
| onNewTaskModeChange?.(newMode) | ||
| }} | ||
| disabled={false} | ||
| title={t("chat:subtasks.selectMode")} | ||
| triggerClassName="inline-flex" | ||
| modeShortcutText="" | ||
| customModes={customModes} | ||
| customModePrompts={customModePrompts} | ||
| disableSearch={true} | ||
| /> | ||
| ) : ( | ||
| <code>{effectiveMode}</code> | ||
| )} | ||
| {t("chat:subtasks.wantsToCreate").split("{mode}")[1]} | ||
| </span> | ||
| </div> | ||
| <div | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -188,6 +188,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |
| const autoApproveTimeoutRef = useRef<NodeJS.Timeout | null>(null) | ||
| const userRespondedRef = useRef<boolean>(false) | ||
| const [currentFollowUpTs, setCurrentFollowUpTs] = useState<number | null>(null) | ||
| const [selectedNewTaskModes, setSelectedNewTaskModes] = useState<Record<number, string>>({}) | ||
|
|
||
| const clineAskRef = useRef(clineAsk) | ||
| useEffect(() => { | ||
|
|
@@ -709,7 +710,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |
| // after which buttons are shown and we then send an askResponse to the | ||
| // extension. | ||
| const handlePrimaryButtonClick = useCallback( | ||
| (text?: string, images?: string[]) => { | ||
| (text?: string, images?: string[], additionalData?: any) => { | ||
| // Mark that user has responded | ||
| userRespondedRef.current = true | ||
|
|
||
|
|
@@ -730,12 +731,17 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |
| askResponse: "yesButtonClicked", | ||
| text: trimmedInput, | ||
| images: images, | ||
| values: additionalData, | ||
| }) | ||
| // Clear input state after sending | ||
| setInputValue("") | ||
| setSelectedImages([]) | ||
| } else { | ||
| vscode.postMessage({ type: "askResponse", askResponse: "yesButtonClicked" }) | ||
| vscode.postMessage({ | ||
| type: "askResponse", | ||
| askResponse: "yesButtonClicked", | ||
| values: additionalData, | ||
| }) | ||
| } | ||
| break | ||
| case "completion_result": | ||
|
|
@@ -1506,6 +1512,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |
| onBatchFileResponse={handleBatchFileResponse} | ||
| onFollowUpUnmount={handleFollowUpUnmount} | ||
| isFollowUpAnswered={messageOrGroup.ts === currentFollowUpTs} | ||
| onNewTaskModeChange={(mode: string) => { | ||
| setSelectedNewTaskModes((prev) => ({ ...prev, [messageOrGroup.ts]: mode })) | ||
| }} | ||
| editable={ | ||
| messageOrGroup.type === "ask" && | ||
| messageOrGroup.ask === "tool" && | ||
|
|
@@ -1912,7 +1921,26 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |
| appearance="primary" | ||
| disabled={!enableButtons} | ||
| className={secondaryButtonText ? "flex-1 mr-[6px]" : "flex-[2] mr-0"} | ||
| onClick={() => handlePrimaryButtonClick(inputValue, selectedImages)}> | ||
| onClick={() => { | ||
| // Check if this is a newTask tool and we have a selected mode | ||
| let additionalData = undefined | ||
| if (lastMessage?.ask === "tool") { | ||
|
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. Is this the best way to handle the type checking? The try-catch for JSON parsing works, but we could extract this logic into a helper function for better reusability and type safety. Also, I notice the state grows unbounded as messages accumulate. Should we consider cleaning up old entries when tasks complete to prevent potential memory issues in long-running sessions?
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. Is this the best way to handle the type checking? The try-catch for JSON parsing works, but we could extract this logic into a helper function for better reusability and type safety. Also, I notice the |
||
| try { | ||
| const tool = JSON.parse(lastMessage.text || "{}") | ||
| if ( | ||
| tool.tool === "newTask" && | ||
| selectedNewTaskModes[lastMessage.ts] | ||
| ) { | ||
| additionalData = { | ||
| selectedMode: selectedNewTaskModes[lastMessage.ts], | ||
| } | ||
| } | ||
| } catch (_e) { | ||
| // Ignore parse errors | ||
| } | ||
| } | ||
| handlePrimaryButtonClick(inputValue, selectedImages, additionalData) | ||
| }}> | ||
| {primaryButtonText} | ||
| </VSCodeButton> | ||
| </StandardTooltip> | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Nice addition of the getter! Consider adding a JSDoc comment to document what this getter returns and when it's used. This would help future maintainers understand the askResponseValues flow better.