Be less aggressive about removing terminal in agents app#312873
Be less aggressive about removing terminal in agents app#312873anthonykim1 merged 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts terminal lifecycle management in the Agents (sessions) window so terminals aren’t aggressively closed when a session is replaced/archived but another live session still shares the same working directory (fixing the “terminal auto-closes when first opened” behavior reported in #312872).
Changes:
- Update terminal cleanup logic to only close terminals for archived/removed sessions when no other live session still owns the same cwd.
- Add regression tests covering session replacement, shared-cwd archiving, and removal behavior.
- Extend test session factory to support custom
sessionIds for replacement scenarios.
Show a summary per file
| File | Description |
|---|---|
src/vs/sessions/contrib/terminal/browser/sessionsTerminalContribution.ts |
Changes terminal cleanup to be cwd-ownership-aware using current live sessions. |
src/vs/sessions/contrib/terminal/test/browser/sessionsTerminalContribution.test.ts |
Adds tests for replace/shared-cwd/archive/remove scenarios; enhances test session creation with sessionId. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 2
Co-authored-by: Copilot <copilot@github.com>
|
@anthonykim1 what are the repro steps here? Does it always happen? |
|
@meganrogge On the buggy scenario that terminal would close immediately. Seen @TylerLeonhardt repro and I believe he also had cli and claude sessions open in agents app and tried to spawn first terminal. Edit: couldnt constant repro either. Like in the TODO, Long term we should have better association of terminal with session or remove closing terminal. |
|
|
||
| this._register(this._sessionsManagementService.onDidChangeSessions(e => { | ||
| for (const session of [...e.removed, ...e.changed.filter(s => s.isArchived.get())]) { | ||
| const archivedChanged = e.changed.filter(s => s.isArchived.get()); |
There was a problem hiding this comment.
As we discussed, this seems like a good stop gap solution, but the initial implementation should have been better here - using IDs and a map to keep track of session terminals. cwd is not good enough as this bug proves.
Since Ben S is out, we should fix it.
Resolves: #312872
Screen.Recording.2026-04-27.at.11.58.35.AM.mov
/cc @TylerLeonhardt
Agents app code has this logic where terminals are forced to close in conditions like archiving/changing sessions based on cwd. Removal was based on cwd, but also cwd was used to share terminal for multiple session which doesn't make sense to me..
I think it's fair to not have such removal, or figure out better way to associate particular session with particular terminal?
Even for associating particular session with particular terminal, user always has to freedom to cd in/out of particular directory so labeling a terminal to a particular session seems to aggressive.