Conversation
There was a problem hiding this comment.
Pull request overview
Updates the Sessions “harness/session type” picker so it can still open on the New Session screen and display all provider-defined session types, with unavailable ones shown as disabled (addressing #308675).
Changes:
- Fetch both (a) provider-wide session types and (b) session-supported session types, and use them to render a full list with disabled entries.
- Change picker open/visibility logic to be based on provider session types rather than only the supported subset.
Show a summary per file
| File | Description |
|---|---|
| src/vs/sessions/contrib/chat/browser/sessionTypePicker.ts | Builds the picker list from provider session types and disables unsupported ones so the dropdown still opens even when only one option is selectable. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/vs/sessions/contrib/chat/browser/sessionTypePicker.ts:97
- Now that the picker can open even when only one session type is supported (by showing other provider types as disabled), users can click the already-checked/enabled item. That will still trigger the delegate's
onSelectpath and callsessionsManagementService.setSessionType(...), which can reject/throw for providers that don't support changing session type (e.g. CopilotChatSessionsProvider throws "Session type cannot be changed"). Consider preventing selection of the current type (or making it a no-op) and handling the returned promise to avoid unhandled rejections.
const items: IActionListItem<ISessionType>[] = this._allProviderSessionTypes.map(type => ({
kind: ActionListItemKind.Action,
label: type.label,
group: { title: '', icon: type.icon },
disabled: !supportedTypeIds.has(type.id),
item: type.id === this._sessionType ? { ...type, checked: true } : type,
}));
- Files reviewed: 1/1 changed files
- Comments generated: 2
| this._register(autorun(reader => { | ||
| const session = this.sessionsManagementService.activeSession.read(reader); | ||
| if (session) { | ||
| this._sessionTypes = this.sessionsManagementService.getSessionTypes(session); | ||
| this._supportedSessionTypes = this.sessionsManagementService.getSessionTypes(session); | ||
| const provider = this.sessionsProvidersService.getProvider(session.providerId); | ||
| this._allProviderSessionTypes = provider ? [...provider.sessionTypes] : []; | ||
| this._sessionType = session.sessionType; | ||
| } else { | ||
| this._sessionTypes = []; | ||
| this._supportedSessionTypes = []; | ||
| this._allProviderSessionTypes = []; | ||
| this._sessionType = undefined; | ||
| } | ||
| this._updateTriggerLabel(); |
There was a problem hiding this comment.
The list of _allProviderSessionTypes is derived from sessionsProvidersService.getProvider(...), but this is only recomputed when activeSession changes. If providers register/unregister after the active session is set (there is existing code handling provider-registration startup races elsewhere), this picker can remain hidden or stale until the session changes again. Consider subscribing to sessionsProvidersService.onDidChangeProviders (or another observable/event) to refresh _allProviderSessionTypes (and _supportedSessionTypes) for the current active session when providers change.
| const supportedTypeIds = new Set(this._supportedSessionTypes.map(t => t.id)); | ||
|
|
||
| const items: IActionListItem<ISessionType>[] = this._allProviderSessionTypes.map(type => ({ | ||
| kind: ActionListItemKind.Action, | ||
| label: type.label, | ||
| group: { title: '', icon: type.icon }, | ||
| disabled: !supportedTypeIds.has(type.id), | ||
| item: type.id === this._sessionType ? { ...type, checked: true } : type, | ||
| })); |
There was a problem hiding this comment.
This change adds new behavior (show all provider session types and mark unsupported ones as disabled). There are existing unit tests for other pickers under src/vs/sessions/contrib/chat/test/browser/ (e.g. workspace picker), but none for SessionTypePicker. Please add tests that verify: (1) the picker opens when the provider has >1 session type even if only one is supported, (2) unsupported types are marked disabled, and (3) selecting a disabled item does not call setSessionType.
This issue also appears on line 91 of the same file.
fix #308675