Conversation
meganrogge
requested review from
Copilot and
sbatten
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
Adds a context menu to the chat tip widget so users can dismiss individual tips (persisted per-workspace) or disable tips entirely via the chat.tips.enabled setting.
Changes:
- Introduces a new
ChatTipContextmenu and registers “Dismiss This Tip” / “Disable Tips” actions. - Extends
ChatTipServiceto support dismiss/disable operations and emits events so the UI can react immediately. - Adds unit tests covering tip dismissal/disable behavior and event firing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/test/browser/chatTipService.test.ts | Adds tests for dismiss/disable behavior and related events. |
| src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts | Wires tip rendering to the new context-menu-enabled ChatTipContentPart and handles tip hide events. |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatTipContentPart.ts | Implements the tip context menu UI behavior and registers the two menu actions. |
| src/vs/workbench/contrib/chat/browser/chatTipService.ts | Adds dismissal persistence, disable behavior, and events; excludes dismissed tips from selection. |
| src/vs/platform/actions/common/actions.ts | Adds a new MenuId.ChatTipContext for the tip widget context menu. |
| templateData.value.appendChild(tipPart.domNode); | ||
| templateData.elementDisposables.add(tipPart); | ||
| templateData.elementDisposables.add(tipPart.onDidHide(() => { | ||
| tipPart.domNode.remove(); |
There was a problem hiding this comment.
When the tip hides, the code removes the DOM node but does not dispose the ChatTipContentPart. That leaves it subscribed to chatTipService events (and holding onto rendered markdown) for the lifetime of the list item. Consider disposing tipPart in the onDidHide handler (or have ChatTipContentPart dispose itself after firing onDidHide).
| tipPart.domNode.remove(); | |
| tipPart.domNode.remove(); | |
| tipPart.dispose(); |
| this.domNode = $('.chat-tip-widget'); | ||
| this._renderTip(tip); | ||
|
|
||
| this._register(this._chatTipService.onDidDismissTip(() => { | ||
| const nextTip = this._getNextTip(); | ||
| if (nextTip) { | ||
| this._renderTip(nextTip); | ||
| } else { | ||
| this._onDidHide.fire(); | ||
| } | ||
| })); | ||
|
|
||
| this._register(this._chatTipService.onDidDisableTips(() => { | ||
| this._onDidHide.fire(); | ||
| })); | ||
|
|
||
| this._register(dom.addDisposableListener(this.domNode, dom.EventType.CONTEXT_MENU, (e: MouseEvent) => { | ||
| dom.EventHelper.stop(e, true); | ||
| const event = new StandardMouseEvent(dom.getWindow(this.domNode), e); | ||
| this._contextMenuService.showContextMenu({ | ||
| getAnchor: () => event, | ||
| getActions: () => { | ||
| const menu = this._menuService.getMenuActions(MenuId.ChatTipContext, this._contextKeyService); | ||
| return getFlatContextMenuActions(menu); | ||
| }, | ||
| }); | ||
| })); |
There was a problem hiding this comment.
The tip widget DOM node isn’t focusable (div with no tabIndex), so keyboard-only users can’t reach it to open the context menu (e.g. via the Context Menu key / Shift+F10). Consider making .chat-tip-widget focusable (and optionally adding an ARIA label/role) so the new actions are accessible without a mouse.
fixes #293507
Adds a right-click context menu to the chat tips widget with two actions:
Dismiss this tip(persists the dismissal to workspace storage and cycles to the next eligible tip) andDisable Tips(turns off thechat.tips.enabled setting).The intent is: