-
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
Conversation
- Added ModeSelector component to newTask tool approval UI in ChatRow - Users can now click on the mode to select a different one before approving - Selected mode is passed through the askResponse flow to the backend - Modified Task.ts to store and provide access to askResponseValues - Updated newTaskTool.ts to use the user-selected mode if provided - Added comprehensive tests for the new functionality - Added translation key for mode selector tooltip Fixes #6706
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.
I reviewed my own code and found it acceptable. The bar was low.
| onClick={() => { | ||
| // Check if this is a newTask tool and we have a selected mode | ||
| let additionalData = undefined | ||
| if (lastMessage?.ask === "tool") { |
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 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?
| public lastMessageTs?: number | ||
|
|
||
| // Getter for askResponseValues to allow tools to access it | ||
| get getAskResponseValues(): Record<string, any> | undefined { |
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.
| <span style={{ fontWeight: "bold", display: "flex", alignItems: "center", gap: "4px" }}> | ||
| {t("chat:subtasks.wantsToCreate").split("{mode}")[0]} | ||
| {message.type === "ask" ? ( | ||
| <ModeSelector |
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.
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.
|
|
||
| // 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 () => { |
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.
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.
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.
I reviewed my own code and found it acceptable. The bar was low.
| onClick={() => { | ||
| // Check if this is a newTask tool and we have a selected mode | ||
| let additionalData = undefined | ||
| if (lastMessage?.ask === "tool") { |
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 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 selectedNewTaskModes 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?
| public lastMessageTs?: number | ||
|
|
||
| // Getter for askResponseValues to allow tools to access it | ||
| get getAskResponseValues(): Record<string, any> | undefined { |
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.
| <span style={{ fontWeight: "bold", display: "flex", alignItems: "center", gap: "4px" }}> | ||
| {t("chat:subtasks.wantsToCreate").split("{mode}")[0]} | ||
| {message.type === "ask" ? ( | ||
| <ModeSelector |
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.
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.
|
|
||
| // 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 () => { |
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.
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.
| "defaultResult": "Bitte fahre mit der nächsten Aufgabe fort.", | ||
| "completionInstructions": "Teilaufgabe abgeschlossen! Du kannst die Ergebnisse überprüfen und Korrekturen oder nächste Schritte vorschlagen. Wenn alles gut aussieht, bestätige, um das Ergebnis an die übergeordnete Aufgabe zurückzugeben." | ||
| "completionInstructions": "Teilaufgabe abgeschlossen! Du kannst die Ergebnisse überprüfen und Korrekturen oder nächste Schritte vorschlagen. Wenn alles gut aussieht, bestätige, um das Ergebnis an die übergeordnete Aufgabe zurückzugeben.", | ||
| "selectMode": "Klicken Sie, um einen anderen Modus für diese Unteraufgabe auszuwählen" |
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.
Typographical note: The translation uses "Unteraufgabe" in 'selectMode' while the other related keys (like 'completionContent' and 'resultContent') use "Teilaufgabe". Consider if this inconsistency is intentional, or if it should be standardized for clarity.
| "selectMode": "Klicken Sie, um einen anderen Modus für diese Unteraufgabe auszuwählen" | |
| "selectMode": "Klicken Sie, um einen anderen Modus für diese Teilaufgabe auszuwählen" |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
| "defaultResult": "कृपया अगले कार्य पर जारी रखें।", | ||
| "completionInstructions": "उपकार्य पूर्ण! आप परिणामों की समीक्षा कर सकते हैं और सुधार या अगले चरण सुझा सकते हैं। यदि सब कुछ ठीक लगता है, तो मुख्य कार्य को परिणाम वापस करने के लिए पुष्टि करें।" | ||
| "completionInstructions": "उपकार्य पूर्ण! आप परिणामों की समीक्षा कर सकते हैं और सुधार या अगले चरण सुझा सकते हैं। यदि सब कुछ ठीक लगता है, तो मुख्य कार्य को परिणाम वापस करने के लिए पुष्टि करें।", | ||
| "selectMode": "इस उप-कार्य के लिए एक अलग मोड चुनने के लिए क्लिक करें" |
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.
Typographical consistency: The new string uses "उप-कार्य" with a hyphen, while other related keys (e.g., "completionContent") use "उपकार्य" without a hyphen. Consider using a consistent spelling for term across translations.
| "selectMode": "इस उप-कार्य के लिए एक अलग मोड चुनने के लिए क्लिक करें" | |
| "selectMode": "इस उपकार्य के लिए एक अलग मोड चुनने के लिए क्लिक करें" |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
| "defaultResult": "Per favore continua con la prossima attività.", | ||
| "completionInstructions": "Sottoattività completata! Puoi rivedere i risultati e suggerire correzioni o prossimi passi. Se tutto sembra a posto, conferma per restituire il risultato all'attività principale." | ||
| "completionInstructions": "Sottoattività completata! Puoi rivedere i risultati e suggerire correzioni o prossimi passi. Se tutto sembra a posto, conferma per restituire il risultato all'attività principale.", | ||
| "selectMode": "Clicca per selezionare una modalità diversa per questo sottocompito" |
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.
Typographical note: There's an inconsistency in the terminology used. Earlier strings use "sottoattività" (e.g., in 'completionContent' and 'resultContent'), but here "selectMode" uses "sottocompito". Consider using a consistent term across keys, unless this variation is intended.
| "selectMode": "Clicca per selezionare una modalità diversa per questo sottocompito" | |
| "selectMode": "Clicca per selezionare una modalità diversa per questa sottoattività" |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
| "defaultResult": "請繼續下一個工作。", | ||
| "completionInstructions": "子工作已完成!您可以檢閱結果並提出修正或下一步建議。如果一切看起來良好,請確認以將結果傳回主工作。" | ||
| "completionInstructions": "子工作已完成!您可以檢閱結果並提出修正或下一步建議。如果一切看起來良好,請確認以將結果傳回主工作。", | ||
| "selectMode": "點擊為此子任務選擇不同的模式" |
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.
Typographical note: The new key uses '子任務' while 'completionInstructions' uses '子工作'. Consider standardizing the terminology unless the distinction is intentional.
| "selectMode": "點擊為此子任務選擇不同的模式" | |
| "selectMode": "點擊為此子工作選擇不同的模式" |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
|
This doesn't work properly, I'll close this and leave the issue unassigned in case anyone wants to work on it |
This PR implements the feature requested in #6706 to allow users to change the mode when the orchestrator requests to create a subtask.
Changes Made
Frontend (webview-ui)
ModeSelectorcomponent instead of static mode text for newTask toolsselectModekey to the English translation file for the mode selector tooltipBackend (src)
handleWebviewAskResponseto accept and storevaluesparameteraskResponseValuesproperty andgetAskResponseValuesgetter methodTests
newTaskTool.spec.tsto verify:How It Works
valuesfieldTesting
All existing tests pass, and new tests have been added to cover the new functionality.
Fixes #6706
Important
Allows users to change mode when approving orchestrator subtasks, with frontend and backend updates and new tests.
ChatRow.tsx: Replaces static mode text withModeSelectorfor newTask tools.ChatView.tsx: Manages state for mode selections by timestamp and passes selected mode on approval.selectModekey to English translation for mode selector tooltip.Task.ts: ModifieshandleWebviewAskResponseto acceptvaluesparameter; addsaskResponseValuesproperty andgetAskResponseValuesgetter.newTaskTool.ts: Checks for user-selected mode inaskResponseValuesand uses it instead of original mode.newTaskTool.spec.tsto verify user-selected mode usage, fallback to original mode, and handling of invalid selections.This description was created by
for 8bedb73. You can customize this summary. It will automatically update as commits are pushed.