-
Notifications
You must be signed in to change notification settings - Fork 29
Handle activation disposes correctly #128
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.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/features/terminal/terminalManager.ts:162
- [nitpick] The error message for the shell execution timeout is not very informative. Consider providing more context about the impact of the timeout.
traceError(`Shell execution timed out: ${command.executable} ${command.args?.join(' ')}`);
src/features/terminal/terminalManager.ts:160
- Ensure that the new timeout behavior is covered by tests.
let timer: NodeJS.Timeout | undefined = setTimeout(() => {
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
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.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/features/terminal/terminalManager.ts:162
- [nitpick] The error message for the shell execution timeout could be more descriptive. Consider including additional context or suggestions for troubleshooting.
traceError(`Shell execution timed out: ${command.executable} ${command.args?.join(' ')}`);
src/features/terminal/terminalManager.ts:385
- The new function
isTaskTerminalshould have test coverage to ensure it behaves as expected.
return terminal.name.toLowerCase().includes('task');
Tip: Turn on automatic Copilot reviews for this repository to get quick feedback on every pull request. Learn more
|
If the changes appear safe, you can manually trigger the pipeline by commenting |
Fixes #127