Add tooltip to chat context item #288418
Add tooltip to chat context item #288418
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds tooltip support to chat context items by introducing an optional tooltip field of type MarkdownString. The tooltip displays when users hover over context items in the chat UI, providing additional information beyond the label.
Changes:
- Added
tooltip?: MarkdownStringfield to the VS Code API'sChatContextIteminterface - Added
tooltip?: IMarkdownStringto internal interfaces for chat context items and variable entries - Implemented tooltip rendering in attachment widgets using
IHoverService.setupDelayedHover
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vscode-dts/vscode.proposed.chatContextProvider.d.ts | Added tooltip?: MarkdownString field to the proposed API's ChatContextItem interface |
| src/vs/workbench/contrib/chat/common/contextContrib/chatContext.ts | Added tooltip?: IMarkdownString to IChatContextItem interface |
| src/vs/workbench/contrib/chat/common/attachments/chatVariableEntries.ts | Added tooltip?: IMarkdownString to StringChatContextValue and IChatRequestStringVariableEntry interfaces |
| src/vs/workbench/contrib/chat/browser/contextContrib/chatContextService.ts | Updated service to propagate tooltip values during context resolution |
| src/vs/workbench/contrib/chat/browser/attachments/implicitContextAttachment.ts | Implemented tooltip rendering for implicit context attachments using conditional hover setup |
| src/vs/workbench/contrib/chat/browser/attachments/chatAttachmentWidgets.ts | Added tooltip rendering for default attachment widgets with updateTooltip method |
| src/vs/workbench/api/common/extHostChatContext.ts | Added type conversion for tooltip field using MarkdownString.from() converter |
| appearance: { showPointer: true }, | ||
| })); | ||
| } else if (title) { | ||
| this._register(this.hoverService.setupManagedHover(getDefaultHoverDelegate('element'), this.domNode, title)); |
There was a problem hiding this comment.
The hover should be registered with renderDisposables instead of this._register() to ensure it's cleaned up when the attachment is re-rendered. The markdown tooltip hover on line 141 is correctly registered with renderDisposables, but this one uses this._register(), which won't be disposed when render() is called again.
| this._register(this.hoverService.setupManagedHover(getDefaultHoverDelegate('element'), this.domNode, title)); | |
| this.renderDisposables.add(this.hoverService.setupManagedHover(getDefaultHoverDelegate('element'), this.domNode, title)); |
| }); | ||
| } | ||
|
|
||
| public updateTooltip(tooltip: IMarkdownString | undefined): void { |
There was a problem hiding this comment.
The updateTooltip method is declared as public but is never called from outside the class. Consider making it private since it's not part of the public API and there's no evidence it needs to be exposed externally.
| public updateTooltip(tooltip: IMarkdownString | undefined): void { | |
| private updateTooltip(tooltip: IMarkdownString | undefined): void { |
| this._register(this.hoverService.setupManagedHover(getDefaultHoverDelegate('element'), this.domNode, title)); | ||
| if (markdownTooltip || title) { | ||
| this.renderDisposables.add(this.hoverService.setupDelayedHover(this.domNode, { | ||
| content: markdownTooltip! ?? title!, |
There was a problem hiding this comment.
The non-null assertion on title is unnecessary here. Since the guard condition checks if (markdownTooltip || title), we know at least one is truthy, and the nullish coalescing operator will only evaluate title if markdownTooltip is nullish. Consider simplifying to content: markdownTooltip ?? title! or better yet, content: (markdownTooltip ?? title)! to make the intent clearer.
| content: markdownTooltip! ?? title!, | |
| content: (markdownTooltip ?? title)!, |
Part of #280658