Reuse PermissionPickerActionItem for agent host auto-approve picker#311263
Reuse PermissionPickerActionItem for agent host auto-approve picker#311263roblourens merged 8 commits intomainfrom
Conversation
Previously the agent host had its own custom auto-approve picker that
duplicated most of the workbench PermissionPickerActionItem widget. This
refactor reuses the workbench widget when the active session's
autoApprove session-config property uses the well-known schema (string
type, enum subset of {default, autoApprove, autopilot} containing
'default'). Sessions that match get the workbench widget; non-conforming
agents fall back to the existing generic per-property picker.
- Adds AgentHostPermissionPickerDelegate and a thin
AgentHostPermissionPickerActionItem subclass that toggles its
visibility reactively as the active session changes.
- Generalizes the warning/info color rules in chat.css so they live
with the widget rather than being scoped to .chat-secondary-toolbar.
- Groups all agent-host-only files under
src/vs/sessions/contrib/chat/browser/agentHost/.
- Fixes a regression where the picker would stay hidden after
navigating back to the new-chat view; the IActionViewItemService
factory only runs once per render, so visibility must be reactive
rather than gated at construction.
(Written by Copilot)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the Agents window’s agent-host auto-approve UI to reuse the workbench PermissionPickerActionItem when the active session’s autoApprove session-config property matches a well-known schema, reducing duplicated picker logic and aligning UX with the workbench chat picker.
Changes:
- Introduces
AgentHostPermissionPickerDelegate+AgentHostPermissionPickerActionItemto bridge agent-host session-config (autoApprove) into the workbench permission picker and reactively show/hide based on the active session. - Updates styling so warning/info coloring is owned by the shared chat picker styles, and adds sessions-specific sizing overrides for the reused widget.
- Reorganizes agent-host-only chat files under
src/vs/sessions/contrib/chat/browser/agentHost/and updates side-effect imports accordingly; adds unit tests for schema detection and reactivity.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/browser/widget/media/chat.css | Broadens warning/info coloring rules for chat picker action labels. |
| src/vs/sessions/sessions.common.main.ts | Updates side-effect import path after agent-host file regrouping. |
| src/vs/sessions/contrib/remoteAgentHost/browser/remoteAgentHost.contribution.ts | Updates side-effect import path for the model picker after regrouping. |
| src/vs/sessions/contrib/chat/test/browser/agentHost/agentHostPermissionPickerActionItem.test.ts | Adds unit tests for delegate mapping, reactivity, and schema predicate. |
| src/vs/sessions/contrib/chat/browser/media/agentHostSessionConfigPicker.css | Removes duplicated warning/info styles and adds sessions-specific sizing overrides for the reused picker widget. |
| src/vs/sessions/contrib/chat/browser/agentHost/agentHostSessionConfigPicker.ts | Skips autoApprove in generic picker when schema is well-known and registers the new permission picker action item factory for relevant menus. |
| src/vs/sessions/contrib/chat/browser/agentHost/agentHostPermissionPickerActionItem.ts | Implements the delegate + action view item to reuse the workbench permission picker and react to active session changes. |
| src/vs/sessions/contrib/chat/browser/agentHost/agentHostModelPicker.ts | Adjusts relative imports after folder regrouping (no functional behavior change). |
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 3
| .chat-input-picker-item .action-label.warning { | ||
| color: var(--vscode-problemsWarningIcon-foreground); | ||
| } | ||
|
|
||
| .interactive-session .chat-secondary-toolbar .chat-input-picker-item .action-label.warning .codicon { | ||
| .chat-input-picker-item .action-label.warning .codicon { | ||
| color: var(--vscode-problemsWarningIcon-foreground) !important; | ||
| } | ||
|
|
||
| .interactive-session .chat-secondary-toolbar .chat-input-picker-item .action-label.info { | ||
| .chat-input-picker-item .action-label.info { | ||
| color: var(--vscode-problemsInfoIcon-foreground); | ||
| } | ||
|
|
||
| .interactive-session .chat-secondary-toolbar .chat-input-picker-item .action-label.info .codicon { | ||
| .chat-input-picker-item .action-label.info .codicon { | ||
| color: var(--vscode-problemsInfoIcon-foreground) !important; |
There was a problem hiding this comment.
The new .chat-input-picker-item .action-label.warning/info selectors are less specific than the existing .interactive-session .chat-*-toolbar .chat-input-picker-item .action-label rules that set color: var(--vscode-icon-foreground), so the warning/info foreground colors will not apply in the toolbars (the more specific rule wins). Consider increasing specificity (e.g. scope to .interactive-session .chat-input-picker-item ...) or otherwise ensure these colors override the toolbar defaults.
See below for a potential fix:
.interactive-session .chat-input-picker-item .action-label.warning {
color: var(--vscode-problemsWarningIcon-foreground);
}
.interactive-session .chat-input-picker-item .action-label.warning .codicon {
color: var(--vscode-problemsWarningIcon-foreground) !important;
}
.interactive-session .chat-input-picker-item .action-label.info {
color: var(--vscode-problemsInfoIcon-foreground);
}
.interactive-session .chat-input-picker-item .action-label.info .codicon {
| /* The agent-host auto-approve picker reuses the workbench PermissionPickerActionItem widget. Override the surrounding .new-chat-in-session toolbar rules (which are intentionally smaller for the sessions-custom pickers) so this widget matches the regular workbench chat-input-toolbar styling. */ | ||
| .sessions-agent-host-permission-picker .chat-input-picker-item .action-label { | ||
| height: 16px; | ||
| padding: 3px 1px 3px 7px; | ||
| font-size: 13px; | ||
| display: flex; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .sessions-chat-agent-host-config .action-label.info .codicon { | ||
| color: var(--vscode-problemsInfoIcon-foreground) !important; | ||
| .sessions-agent-host-permission-picker .chat-input-picker-item .action-label span + .chat-input-picker-label { | ||
| margin-left: 2px; |
There was a problem hiding this comment.
.sessions-agent-host-permission-picker is added to the same action-item container that already gets the chat-input-picker-item class (via ChatInputPickerActionViewItem.render). The selector .sessions-agent-host-permission-picker .chat-input-picker-item .action-label therefore won’t match (it expects a descendant .chat-input-picker-item). Update the selector to target the same element (e.g. .sessions-agent-host-permission-picker.chat-input-picker-item .action-label) so the sizing overrides actually apply.
| const onDidChangeProviders = new Emitter<ISessionsProvidersChangeEvent>(); | ||
| const sessionsProvidersService = new (class extends mock<ISessionsProvidersService>() { | ||
| override readonly onDidChangeProviders = onDidChangeProviders.event; | ||
| override getProviders(): ISessionsProvider[] { return [provider as unknown as ISessionsProvider]; } | ||
| override getProvider<T extends ISessionsProvider>(id: string): T | undefined { | ||
| return id === provider.id ? (provider as unknown as T) : undefined; | ||
| } | ||
| })(); |
There was a problem hiding this comment.
onDidChangeProviders is an Emitter that is never disposed. Since this suite uses ensureNoDisposablesAreLeakedInTestSuite(), this can cause leak-detection failures. Please dispose the emitter (e.g. add it to the leak-tracking store or return it from setup and dispose alongside the provider/delegate).
…rmissionPickerActionItem (workbench) Both pickers now render in the same context (new-chat-page action bar), so the previously-needed CSS specificity overrides (icon/font sizing, padding, color rule un-scoping in chat.css) all go away. Changes: - Add IPermissionPickerDelegate to permissionPicker.ts: optional currentLevel observable, optional isApplicable observable, setLevel. - Refactor PermissionPicker to take a delegate. When currentLevel is provided, the picker label tracks it reactively. When isApplicable is provided, the picker hides itself when false. - Add CopilotPermissionPickerDelegate (writes through to the active CopilotChatSessionsProvider session). Preserves today's behavior. - Move AgentHostPermissionPickerDelegate to its own file under agentHost/, retargeted at the new interface (currentLevel, isApplicable, setLevel). - agentHostSessionConfigPicker.ts now constructs PermissionPicker with the agent host delegate and wraps in PickerActionViewItem. - Delete agentHostPermissionPickerActionItem.ts (the workbench widget subclass) and its CSS overrides. - Revert chat.css warning/info color un-scoping (no longer needed since agent host doesn't use the workbench widget anymore). - Rename the test file to match the new product file. Tests cover the delegate's permission-level mapping, isApplicable reactivity, and isWellKnownAutoApproveSchema. (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…er for NewSessionControl Restore the contextual split: agent host now uses each widget where it's expected to render. The new-chat page (NewSessionControl) keeps the sessions PermissionPicker (matches surrounding sessions pickers); the running chat widget (ChatInputSecondary) uses the workbench PermissionPickerActionItem (matches the rest of the chat-input secondary toolbar that the extension-host CLI already uses). Both share AgentHostPermissionPickerDelegate. To make the same delegate satisfy both consumers, rename its members to currentPermissionLevel/setPermissionLevel (matching the workbench's IPermissionPickerDelegate). (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both `agentHostPermissionPickerDelegate.ts` and `agentHostSessionConfigPicker.ts` had identical `'autoApprove'` string constants (one called `AUTO_APPROVE_SESSION_CONFIG_PROPERTY`, the other `AUTO_APPROVE_PROPERTY`). Standardize on the shorter name and import it in the picker. Also drops a now-unused `KNOWN_AUTO_APPROVE_VALUES` set that was left behind in the picker file. (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/vs/sessions/contrib/chat/browser/agentHost/agentHostSessionConfigPicker.ts:286
- This comment says the workbench
PermissionPickerActionItemregistered forMenus.NewSessionControlhandles the well-knownautoApprovecase, but this file actually wiresMenus.NewSessionControlto the sessionsPermissionPicker(and usesPermissionPickerActionItemonly forChatInputSecondary). Please update the comment to match the actual implementation to avoid confusion when maintaining the menu wiring.
src/vs/sessions/contrib/chat/browser/agentHost/agentHostSessionConfigPicker.ts:476 - In the running chat widget (
MenuId.ChatInputSecondary), the factory always createsAgentHostPermissionPickerActionItem, which then hides itself whendelegate.isApplicableis false. That means non-conforming agent-host sessions no longer have any in-chat UI to changeautoApprove(previously the generic per-property picker covered this), so they don't actually “fall back” in running sessions. Consider reinstating a fallback view item for non-well-known schemas (e.g., keep a lightweight generic picker forautoApprovewhenisApplicableis false, or dynamically choose between the workbench permission picker and the generic picker based on the schema).
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Dispose `onDidChangeProviders` Emitter in delegate test setup so the suite-level leak detector is happy. Refactor `setup()` to take the leak-tracking store and register all created disposables itself, so individual tests don't repeat the boilerplate. - Fix corrupted JSDoc on `CopilotPermissionPickerDelegate` (sentence was mangled across an inline-code span). (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…approve-picker-widget
The `slot` local in `render()` is in scope for the `autorun` closures, so storing it on `this` is unnecessary. Removing the field also drops the needless null-check inside the visibility autorun. (Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously the agent host had its own custom auto-approve picker that duplicated most of the workbench
PermissionPickerActionItemwidget. This refactor reuses the workbench widget when the active session'sautoApprovesession-config property uses the well-known schema (string type, enum subset of{default, autoApprove, autopilot}containingdefault). Sessions that match get the workbench widget; non-conforming agents fall back to the existing generic per-property picker.Changes
AgentHostPermissionPickerDelegateand a thinAgentHostPermissionPickerActionItemsubclass that toggles its visibility reactively as the active session changes.chat.cssso they live with the widget rather than being scoped to.chat-secondary-toolbar.src/vs/sessions/contrib/chat/browser/agentHost/.IActionViewItemServicefactory only runs once per render, so visibility must be reactive rather than gated at construction.Testing
(Written by Copilot)
Fix #311118