-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(chat): prevent clearing input state for resume task #6361
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.
I've reviewed this PR that addresses issue #4587 regarding input preservation when resuming tasks. The core fix is correct, but there are some important considerations:
Critical Issues (Must Fix):
- Inconsistent behavior between primary and secondary buttons: The primary button (Resume Task) now preserves input at https://github.com/RooCodeInc/Roo-Code/blob/hotfix-4587/webview-ui/src/components/chat/ChatView.tsx#L737-L741, but the secondary button (Terminate) still clears it unconditionally. This creates inconsistent UX where users lose their input depending on which button they click. Should the secondary button also preserve input state for resume_task scenarios?
Important Suggestions (Should Consider):
-
Missing test coverage: The changes modify critical user interaction logic but don't include tests to verify the new behavior. Could you add tests to ensure input preservation works correctly for resume_task scenarios?
-
Incomplete PR template: The PR description lacks key sections like test procedure details. Could you provide clear steps for reviewers to verify the fix manually?
Minor Improvements (Nice to Have):
-
Code organization: The conditional logic at https://github.com/RooCodeInc/Roo-Code/blob/hotfix-4587/webview-ui/src/components/chat/ChatView.tsx#L737 could be extracted into a helper function like
shouldPreserveInputState(clineAsk)to improve readability and make the intent clearer. -
Consider edge cases: What happens if a user has both text input and selected images when resuming a task? The current implementation preserves both, but this behavior should be explicitly documented or tested.
The core fix correctly addresses the reported issue by preventing input clearing for resume_task scenarios. The implementation is straightforward and follows the existing code patterns.
|
@roomote-agent Termination means exiting the current task, so clearing the input box is reasonable. |
|
Thanks for the clarification @NaccOll! You're absolutely right that termination means exiting the current task, so clearing the input box for the Terminate button is indeed reasonable behavior. I'll review the current implementation to ensure the logic is consistent with this understanding - Resume should preserve input (for continuing the task) while Terminate should clear it (for ending the task). Let me examine the code changes. |
Related GitHub Issue
Closes: #4587
Roo Code Task Context (Optional)
Description
stop clear message textarea when clicking resume task
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch