Switch over to command for PR chat response type#293161
Conversation
There was a problem hiding this comment.
Pull request overview
This PR switches the ChatResponsePullRequestPart from using a URI to using a Command for better control over how pull request links are handled in chat responses. This addresses issue #290997 where the cloud agent's PR links were showing confusing long URIs instead of user-friendly messages.
Changes:
- Deprecated the
uriproperty in favor of acommandproperty in the proposed API and internal interfaces - Updated the UI rendering to execute commands instead of opening URIs directly via the opener service
- Added backward compatibility logic to handle cases where only a URI is provided
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vscode-dts/vscode.proposed.chatParticipantAdditions.d.ts | Deprecated uri property and added command property to ChatResponsePullRequestPart API |
| src/vs/workbench/contrib/chat/common/chatService/chatService.ts | Updated internal IChatPullRequestContent interface to match API changes |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatPullRequestContentPart.ts | Changed from IOpenerService to ICommandService for executing commands when PR link is clicked |
| src/vs/workbench/api/common/extHostTypes.ts | Updated constructor to accept either URI or Command, with backward compatibility logic |
| src/vs/workbench/api/common/extHostTypeConverters.ts | Added command conversion logic with fallback for restored sessions |
| src/vs/workbench/contrib/chat/test/browser/accessibility/chatResponseAccessibleView.test.ts | Added command field to test data |
Comments suppressed due to low confidence (1)
src/vs/workbench/api/common/extHostTypes.ts:3330
- The toJSON method is missing the 'linkTag' and 'command' fields. For proper serialization/deserialization, all properties should be included. Add 'linkTag' and 'command' to the returned object.
toJSON() {
return {
$mid: MarshalledId.ChatResponsePullRequestPart,
uri: this.uri,
title: this.title,
description: this.description,
author: this.author
};
| titleLink.textContent = `${this.pullRequestContent.title} - ${this.pullRequestContent.author}`; | ||
| titleLink.href = this.pullRequestContent.uri.toString(); | ||
| if (this.pullRequestContent.uri) { | ||
| titleLink.href = this.pullRequestContent.uri?.toString(); |
There was a problem hiding this comment.
The optional chaining operator is redundant here. The condition on line 35 already checks if 'this.pullRequestContent.uri' exists, so using '?.' again on line 36 is unnecessary. Use 'this.pullRequestContent.uri.toString()' instead.
| titleLink.href = this.pullRequestContent.uri?.toString(); | |
| titleLink.href = this.pullRequestContent.uri.toString(); |
| this.uri = uriOrCommand as vscode.Uri; | ||
| this.command = { | ||
| title: 'Open Pull Request', | ||
| command: 'vscode.open', |
There was a problem hiding this comment.
The property name should be 'id' instead of 'command' to match the Command interface definition (which has 'id: string' as the property name for the command identifier). This should be:
id: 'vscode.open',| let command: extHostProtocol.ICommandDto; | ||
| if (!part.command) { | ||
| if (!part.uri) { | ||
| throw new Error('Pull request part must have a command if URI is provided'); |
There was a problem hiding this comment.
The error message is incorrect. It should say "Pull request part must have a URI if command is not provided" (not "if URI is provided"). The logic checks for the absence of both command and URI, which is an error state.
| throw new Error('Pull request part must have a command if URI is provided'); | |
| throw new Error('Pull request part must have a URI if command is not provided'); |
Part of #290997