Fix terminal reuse logic#7157
Conversation
ghost
left a comment
There was a problem hiding this comment.
Thank you for this fix! The changes correctly address the terminal reuse logic issue. The removal of the requiredCwd parameter ensures commands will only execute in terminals with matching directories, preventing the mismatch between expected and actual execution directories.
Review Findings:
✅ What works well:
- The fix correctly removes the problematic "reuse any non-busy terminal" logic
- Test updates properly reflect the API changes
- The core issue of commands executing in unexpected directories is resolved
📝 Suggestions for improvement:
-
Documentation update in TerminalRegistry.ts: The JSDoc comment for
getOrCreateTerminalhas been updated to remove therequiredCwdparameter reference, which is good. Consider adding a note about the terminal reuse behavior for extra clarity. -
Consider adding explanatory comment: Where the "Third priority" logic was removed (around line 194 in TerminalRegistry.ts), it might be helpful to add a comment explaining why we no longer reuse any non-busy terminal. This would help future maintainers understand the design decision.
-
Test coverage: The test updates correctly reflect the API change. Consider adding a specific test case that validates terminals with different working directories won't be reused, even when not busy, to ensure the bug doesn't resurface.
Overall, this is a solid fix that addresses the core issue effectively. The changes are minimal and focused, which reduces the risk of introducing new issues.
In the words of Opus:
Important
Fixes terminal reuse logic by removing
requiredCwdparameter, ensuring commands execute in the specified directory.requiredCwdparameter inTerminalRegistry.getOrCreateTerminal().executeCommand.spec.tsto reflect changes in terminal reuse logic.requiredCwdparameter from test cases inexecuteCommand.spec.ts.TerminalRegistry.ts.This description was created by
for 72f8c1a. You can customize this summary. It will automatically update as commits are pushed.