Merge pull request #281589 from microsoft/osortega/various-fixes-prog… #281609
Conversation
…ress Various fixes for session progress
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces various fixes for session progress functionality in the chat feature. The changes improve how session descriptions are displayed during different states of chat operations, particularly for tool invocations and thinking states.
Key Changes
- Enhanced session description generation to prioritize
generatedTitleover other message types for tool invocations - Simplified the logic for determining when to show tool invocation descriptions by removing the state check restriction
- Added support for displaying "Thinking..." status when a thinking part is in progress
- Extracted duplicate diff validation logic into a reusable
hasValidDiffmethod for better code maintainability - Removed fallback to cached session descriptions in the agent sessions model, ensuring fresh data is always used
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/browser/chatSessions.contribution.ts | Updated getSessionDescription to prioritize generatedTitle, simplified conditional logic for tool invocations, and added handling for 'thinking' part kind |
| src/vs/workbench/contrib/chat/browser/agentSessions/agentSessionsViewer.ts | Extracted hasValidDiff helper method to reduce code duplication and improve maintainability |
| src/vs/workbench/contrib/chat/browser/agentSessions/agentSessionsModel.ts | Removed fallback to cached description, ensuring session descriptions are always current |
| } else if (part.kind === 'progressMessage') { | ||
| description = part.content; | ||
| } else if (part.kind === 'thinking') { | ||
| description = localize('chat.sessions.description.thinking', 'Thinking...'); |
There was a problem hiding this comment.
User-visible strings should use double quotes instead of single quotes. Change 'Thinking...' to "Thinking..." to be consistent with the codebase convention that externalized strings use double quotes (see line 972 for an example in the same function).
| const { changes: diff } = session.element; | ||
| if (session.element.status !== ChatSessionStatus.InProgress && diff) { | ||
| if (session.element.status !== ChatSessionStatus.InProgress && diff && this.hasValidDiff(diff)) { | ||
| if (diff instanceof Array ? diff.length > 0 : (diff.files > 0 || diff.insertions > 0 || diff.deletions > 0)) { |
There was a problem hiding this comment.
This line duplicates the logic from the newly extracted hasValidDiff method. The condition on line 164 already calls hasValidDiff(diff), making this redundant check unnecessary. Consider removing this line since the validation is already performed.
See below for a potential fix:
const diffAction = template.elementDisposable.add(new AgentSessionShowDiffAction(session.element));
template.detailsToolbar.push([diffAction], { icon: false, label: true });
Various fixes for session progress
Candidate: #281589