Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the unused “second” model picker action view item implementation and consolidates callers onto a single ModelPickerActionItem, while also tightening a couple of internal-only types in the chat model picker/selection logic.
Changes:
- Delete
modelPickerActionItem2.tsand switch consumers toModelPickerActionItem. - Refactor
ModelPickerActionItemto wrapModelPickerWidgetdirectly (instead of the dropdown/action-widget path). - Make
IModelSelectionContextandModelPickerBadgenon-exported (internal-only).
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/browser/widget/input/modelPickerActionItem2.ts | Removes the unused enhanced model picker action item implementation. |
| src/vs/workbench/contrib/chat/browser/widget/input/modelPickerActionItem.ts | Reimplements the model picker action item as a thin wrapper around ModelPickerWidget. |
| src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts | Updates chat input toolbar to instantiate ModelPickerActionItem (and removes old import). |
| src/vs/sessions/contrib/copilotChatSessions/browser/copilotChatSessionsActions.ts | Switches sessions window model picker to ModelPickerActionItem. |
| src/vs/workbench/contrib/chat/browser/widget/input/chatModelSelectionLogic.ts | Makes model selection context type internal to the module. |
| src/vs/workbench/contrib/chat/browser/widget/input/chatModelPicker.ts | Makes ModelPickerBadge internal to the module. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 1
| override render(container: HTMLElement): void { | ||
| this._pickerWidget.render(container); | ||
| this.element = this._pickerWidget.domNode; | ||
| this._updateTooltip(); | ||
| container.classList.add('chat-input-picker-item'); | ||
| } | ||
|
|
||
| protected override setAriaLabelAttributes(element: HTMLElement): void { | ||
| super.setAriaLabelAttributes(element); | ||
| const modelName = this.currentModel?.metadata.name ?? localize('chat.modelPicker.auto', "Auto"); | ||
| element.ariaLabel = localize('chat.modelPicker.ariaLabel', "Pick Model, {0}", modelName); | ||
| private _getAnchorElement(): HTMLElement { | ||
| if (this.element && getActiveWindow().document.contains(this.element)) { | ||
| return this.element; | ||
| } | ||
| return this.pickerOptions.getOverflowAnchor?.() ?? this.element!; | ||
| } | ||
|
|
||
| protected override renderLabel(element: HTMLElement): IDisposable | null { | ||
| const { name, statusIcon } = this.currentModel?.metadata || {}; | ||
| const domChildren = []; | ||
| public openModelPicker(): void { | ||
| this._showPicker(); | ||
| } | ||
|
|
||
| if (statusIcon) { | ||
| const iconElement = renderIcon(statusIcon); | ||
| domChildren.push(iconElement); | ||
| } | ||
| public show(): void { | ||
| this._showPicker(); | ||
| } | ||
|
|
||
| domChildren.push(dom.$('span.chat-input-picker-label', undefined, name ?? localize('chat.modelPicker.auto', "Auto"))); | ||
| domChildren.push(...renderLabelWithIcons(`$(chevron-down)`)); | ||
| public setEnabled(enabled: boolean): void { | ||
| this._pickerWidget.setEnabled(enabled); | ||
| } |
There was a problem hiding this comment.
ModelPickerActionItem no longer reacts to action.enabled changes: BaseActionViewItem will call updateEnabled() on action change events, but this class doesn’t override it, and render() doesn’t apply the initial enabled state either. As a result the picker can remain interactive/appear enabled even when the backing menu action becomes disabled. Consider overriding updateEnabled() to forward this.action.enabled into this._pickerWidget.setEnabled(...) (and apply it during render()).
There was two action view items. One which we don't use anymore
cc @sandy081