-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: track terminal working directory changes across commands #7568
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
- Add lastUsedTerminal property to Task to persist terminal reference - Update executeCommandTool to use terminal current directory for subsequent commands - Resolve relative paths based on terminal cwd instead of task cwd - Add comprehensive tests for directory tracking scenarios Fixes #7567
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.
Fixed a regression I caused, but at least the tests pass now... unlike my self-esteem.
Review Summary
The implementation correctly addresses the regression reported in #7567. The solution is elegant and maintains backward compatibility.
Suggestions for improvement:
-
Memory management: Consider explicitly setting
lastUsedTerminal = undefinedin the Task'sdispose()method to ensure proper cleanup. -
Type safety: The
lastUsedTerminalproperty could benefit from more explicit optional typing. -
Documentation: Adding JSDoc comments for the new property would help future maintainers understand its purpose and lifecycle.
-
Test coverage: While the tests are comprehensive, consider adding a case for multiple tasks sharing terminals.
Positive observations:
- Clean implementation that properly tracks terminal directory changes
- Excellent test coverage with edge cases
- Proper fallback handling when terminals are closed
- Maintains backward compatibility
Overall, this is a solid fix that addresses the issue effectively.
- Added cleanup of lastUsedTerminal property in Task.dispose() method - Prevents terminal references from being retained after task disposal - Added test to verify proper cleanup on disposal
daniel-lxs
left a comment
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.
LGTM
Add missing lastUsedTerminal property to mock tasks and reset it between tests. Also add taskId to the second mock task which is required by TerminalRegistry. These properties were introduced by the PR for terminal directory tracking.
|
Is the problem that the task's cwd doesn't get updated correctly? The model should be passing the desired cwd in as part of the execute_command call. |
Description
This PR fixes a regression where the TerminalRegistry doesn't track directory changes within tasks. Previously, when a command changed directories (e.g.,
cd subdir), subsequent commands would spawn new terminals instead of reusing the existing terminal with the updated working directory.Problem
The issue occurred because:
cwdtoTerminalRegistry.getOrCreateTerminal()Solution
The fix implements terminal directory tracking by:
lastUsedTerminalproperty to the Task class to persist the terminal referenceexecuteCommandToolto use the terminal's current working directory for subsequent commandsTesting
executeCommandDirectoryTracking.spec.tscovering:cd subdirfollowed by commands)cdcommandsImpact
Fixes #7567
Important
Fixes terminal working directory tracking across commands by adding
lastUsedTerminaltoTaskand updatingexecuteCommandlogic.executeCommandTool.tswhere terminal working directory changes were not tracked, causing new terminals to spawn.lastUsedTerminaltoTaskclass to track terminal reference and working directory.executeCommandto use terminal's current working directory for subsequent commands.executeCommandDirectoryTracking.spec.tsto test directory changes, relative path resolution, and terminal closure scenarios.Task.dispose.test.tsto ensurelastUsedTerminalis cleared to prevent memory leaks.executeCommandTimeout.integration.spec.tsto includelastUsedTerminalproperty.This description was created by
for 33affa5. You can customize this summary. It will automatically update as commits are pushed.