Fix deleting forked session showing wrong confirmation dialog#313205
Fix deleting forked session showing wrong confirmation dialog#313205
Conversation
When clicking X on a sub-session tab, deleteChat checks chatIds.length before finding the specific chat by URI. If the grouping cache doesn't include the forked chat (e.g. due to stale sessionParentId metadata), _getChatIdsInGroup returns only the main chat. The chatIds.length <= 1 check then falls through to deleteSession, which shows 'delete this session?' and deletes the entire session including the worktree. Fix: find the specific chat by URI first. Only fall through to deleteSession if the chat IS found in the group AND it's the last one. Fixes #313148 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes an edge case in the Agents sessions UI where deleting a forked (sub-)chat tab could incorrectly fall through to deleting the entire session (including its worktree) when the multi-chat grouping cache is stale.
Changes:
- Reorders
deleteChatlogic to resolve the specific chat by URI before deciding whether it’s the last chat in the group. - Prevents accidental full-session deletion by returning early when the target chat isn’t found in the current grouping result.
Show a summary per file
| File | Description |
|---|---|
src/vs/sessions/contrib/copilotChatSessions/browser/copilotChatSessionsProvider.ts |
Makes deleteChat identify the chat-by-URI before applying the “last chat => delete session” fallback, avoiding unintended session/worktree deletion under stale grouping. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 1
| @@ -1478,6 +1477,11 @@ export class CopilotChatSessionsProvider extends Disposable implements ISessions | |||
| return; | |||
| } | |||
There was a problem hiding this comment.
The regression fix in deleteChat (returning early when the chat URI isn’t found in the current group) isn’t covered by tests. Given the severity of the original bug (accidentally deleting the whole session/worktree), please add a unit test that calls deleteChat(mainSessionId, forkChatUri) where the fork isn’t grouped under the main session (e.g. due to missing/stale sessionParentId) and asserts that deleteSession is not triggered (model sessions remain unchanged).
When clicking X on a sub-session tab,
deleteChatcheckschatIds.length <= 1before finding the specific chat by URI. If the grouping cache doesn't include the forked chat (e.g. due to stalesessionParentIdmetadata),_getChatIdsInGroupreturns only the main chat. ThechatIds.length <= 1check then falls through todeleteSession, which shows "delete this session?" and deletes the entire session including the worktree.Fix: Find the specific chat by URI first, then check if it's the last chat. Only fall through to
deleteSessionif the chat IS found AND it's the last one. If the chat isn't in the current grouping result, return early instead of accidentally deleting the whole session.Fixes #313148