refactor terminal architecture to address critical issues with the current design #1365
+669
−217
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit combines three related improvements to terminal testing: - Create a reusable function for testing terminal commands with real output - Update tests to properly invoke terminal shell execution handlers - Add microsecond timing to measure execution performance Key improvements: - Added testTerminalCommand function that takes command and expected output - Use child_process.execSync to run real commands and feed output into mock terminal stream - Properly trigger VSCode onDidStartTerminalShellExecution and onDidEndTerminalShellExecution events - Add timeout mechanism to prevent hanging tests - Measure execution time from terminal process creation to command completion - Display both microseconds and milliseconds in test output - Add test for base64 encoded zeros with configurable line count - Increase buffer size for execSync to handle large outputs - Limit output display to avoid cluttering the terminal Signed-off-by: Eric Wheeler <[email protected]>
|
Transformed the TerminalInfo interface into a proper Terminal class and moved it to its own file. This improves code organization and encapsulation by centralizing terminal-related functionality. The change establishes a clearer object model for terminal management, setting the foundation for a more maintainable terminal architecture. All references throughout the codebase have been updated to use the new Terminal class while preserving existing functionality. Tests have been updated and verified to ensure compatibility with the new structure. Signed-off-by: Eric Wheeler <[email protected]>
KJ7LNW
changed the title
added 3 commits
March 3, 2025 21:14
This commit moves the interpretExitCode method from TerminalManager to TerminalProcess class, as part of the terminal refactoring effort. The method is responsible for translating exit codes into detailed results, including signal information. Changes include: - Moved interpretExitCode method to TerminalProcess class - Updated imports in TerminalManager and Cline to reference ExitCodeDetails from TerminalProcess - Added findTerminalIdByVscodeTerminal helper method in TerminalManager - Added comprehensive unit tests for interpretExitCode in TerminalProcess - Tests cover undefined exit codes, normal exit codes (0-127), and signal exit codes (128+) This change improves code organization by placing the exit code interpretation logic closer to where it's primarily used, in the TerminalProcess class. Signed-off-by: Eric Wheeler <[email protected]>
This commit partially addresses the issue of duplicate handler calls by removing an unnecessary instantiation of TerminalManager in registerTerminalActions.ts. Move the getTerminalContents method from TerminalManager to Terminal class as a static method and update all references to use the new location. Signed-off-by: Eric Wheeler <[email protected]>
Replace echo -e with printf command in terminal tests for better portability. - Replace echo -e with printf to ensure consistent behavior across different shell implementations - Not all implementations of echo support the -e flag for interpreting backslash escapes - Using printf provides a more reliable way to handle escape sequences Signed-off-by: Eric Wheeler <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Refactoring the terminal architecture to address critical issues with the current design. See #1364.
This is a work in progress, not in yet intended to be pulled; I am putting it up for reference and commentary.
The existing implementation suffers from inconsistent terminal ownership, and which results in lack of a clear object-oriented model. This leads to even race conditions and event duplication, as well as inconsistent behavior when multiple Roo webviews are active.
Implementation
The refactoring goal:
taskIdownership and stateClinewill interact directly withTerminalRegistryto an available terminal as necessary.Implementation Order
To implement these changes with minimal disruption, the following commit order is recommended:
Rename and Relocate TerminalInfo:
Move interpretExitCode to TerminalProcess:
Move getTerminalContents to Terminal:
Convert Terminal to Class:
Add Terminal Functionality:
Enhance TerminalRegistry:
Move Shell Execution Handlers:
Update Cline Integration:
Remove TerminalManager:
How to Test (ongoing work)
To reproduce the issue that will ultimately be fixed by this PR:
sleep 30in one instanceGet in Touch
My handle on the Roo Code Discord is KJ7LNW. This PR addresses bug #1364.