Customizations modal redesign - #322043
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prototypes a redesigned Plugins experience in the AI Customizations management editor, shifting the default surface toward a card-based “home” while retaining WorkbenchList for search and list-based interactions.
Changes:
- Introduces a card-based plugins home surface, plus a browse/search experience that mixes installed/remote/marketplace results.
- Enhances plugin detail rendering to include provenance/facts, contribution inventory, and inline management actions.
- Updates styling and component fixtures to support the redesigned layouts and new marketplace recommendation signal (
recommendedPlugins).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/test/browser/componentFixtures/sessions/aiCustomizationManagementEditor.fixture.ts | Updates fixture mocks (adds recommendedPlugins) and switches browse-mode activation to the widget API. |
| src/vs/workbench/contrib/chat/browser/aiCustomization/pluginListWidget.ts | Reworks plugin list UI: card home surface, search headers, recommended badges, and marketplace/installed search blending. |
| src/vs/workbench/contrib/chat/browser/aiCustomization/media/aiCustomizationManagement.css | Adds/adjusts CSS for the new plugin list rows, card surfaces, and richer embedded detail layout. |
| src/vs/workbench/contrib/chat/browser/aiCustomization/embeddedAgentPluginDetail.ts | Expands embedded plugin detail with install/enable/disable/uninstall actions, facts, copy path, and grouped contributions. |
| src/vs/workbench/contrib/chat/browser/aiCustomization/aiCustomizationManagementEditor.ts | Wires new embedded-detail events to open contributed artifacts/sections and handle uninstall navigation. |
| src/vs/sessions/AI_CUSTOMIZATIONS.md | Documents the new Plugins section UX and behaviors. |
67e3b40 to
975841b
Compare
SteVen Batten (sbatten)
left a comment
There was a problem hiding this comment.
🤖 AI review verdict: request changes.
I found several functional and accessibility regressions in the redesigned card surfaces, including a recurrence of the MCP node-replacement bug fixed in #330890 and removal of the Settings Sync warning added in #331529. In addition to the inline findings, two required checks are red:
- Compile & Hygiene fails because the new editor title contains an unsupported U+2013 character (inline comment below).
- Screenshots & Tests passes Playwright, capture, and fixture-error checks, but fails because the newly gated blocks-CI hashes were not committed. Please review the visual diffs and update
test/componentFixtures/blocks-ci-screenshots.mdusing the CI-generated hashes.
No high-confidence exploitable security issue was found, but the known MCP regression, keyboard/accessibility failures, migration data-availability regression, and required-check failures make this not ready to merge.
|
Follow-up on the overall AI review verdict: the review was high-signal and the inline findings were substantively valid (with the The blocks-CI screenshot warning was also valid for the reviewed commit. I reviewed the affected customization fixtures locally: 16 selected fixtures render without errors, and 10 dark/light/default/narrow fixtures are stable across three captures. I have not written screenshot hashes from macOS because For bot refinement, it would help to separate durable code findings from transient check-state findings and re-check the latter against the latest PR head before posting. That keeps an otherwise excellent review actionable across a fast-moving branch. |
…lity - Enhance aiCustomizationListWidget and related management editors. - Introduce aiCustomizationPresentation for better UI handling. - Update styles in aiCustomizationManagement.css for consistency. - Improve tests for aiCustomization components to ensure reliability.
- Improve aiCustomizationManagementEditor and associated components - Enhance styling in aiCustomizationManagement and welcome prompt - Update tests for aiCustomizationManagementEditor and welcome page
- Update agent host sessions provider for better integration. - Enhance AI customization management editor and presentation. - Optimize embedded agent plugin and MCP server details. - Clean up CSS for AI customization management. - Adjust tests to reflect changes in AI customization components.
0713a7a to
e1d29dd
Compare
Route failed-turn resume through the provider service and use the existing provider registration helper in its tests.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteVen Batten (sbatten)
left a comment
There was a problem hiding this comment.
🤖 AI second-pass review: request changes.
The findings from the first review were addressed, including the shared composite-list accessibility work, migration disclosure, README render guard/status handling, hygiene, and CSS cleanup. This pass found eight additional issues in the revised paths; the highest priority is a remaining session-backed variant of the MCP node-replacement bug fixed by #330890.
Current CI has 35 passing checks and one pending. The only failure is Screenshots & Tests: fixture tests, screenshot capture, and fixture error checks pass, but the updated blocks-CI screenshot hashes have not been committed.
roblourens
left a comment
There was a problem hiding this comment.
[Experimental performance review bot]
Human-approved experimental performance review.
(Written by Copilot)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preserve live MCP and plugin detail controls, map remote MCP resources correctly, defer marketplace and editor work until visible, scope hook discovery by storage, and make card-list metadata linear.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4c2a49e
Keep Agent Host provider mapping compatible with lightweight providers and accept the reviewed component screenshot baselines generated by CI.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteVen Batten (sbatten)
left a comment
There was a problem hiding this comment.
🤖 Third-pass review complete at current PR head 58b049a911f827443e0eaa195980f123e2f5b71a.
No new findings.
This pass was deliberately limited to changes after the second reviewed head:
4c2a49ed: fixes for the eight second-pass comments;c4a03bb1: CI/screenshot baseline fixes;58b049a9: subsequent merge frommain, reviewing only its three combined conflict resolutions.
All eight second-pass fixes were verified, and no regressions introduced by those fixes were found. Compile/hygiene and the relevant platform/unit checks pass. The refreshed component-fixture run has no Playwright, render, or fixture errors; its current failure is only another blocks-CI screenshot hash mismatch after the latest merge.
Accept the reviewed Agent Host migration and welcome-page hashes generated by the blocks-CI Ubuntu run.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| .map(marketplacePluginToItem); | ||
| this.searchInput.hideMessage(); | ||
| } catch { | ||
| this.marketplaceItems = []; |
There was a problem hiding this comment.
AI Review: Each non-browse search cancels the previous token, but this catch ignores cancellation and request identity. If an older fetch rejects after a newer request succeeds, it clears the newer marketplace results, shows a stale warning, and refilters; clearing the query also leaves pending work active. Cancel delayed and in-flight work when the query clears, and only mutate results or messages when the CTS, query, and browse mode still match the initiating request.
| getActions: () => { | ||
| const state = getPluginEnablementActionState(item.plugin.enablement.get()); | ||
| return [ | ||
| this.renderDisposables.add(new Action(`plugin.${state.isEnabled ? 'exclude' : 'include'}AlternateScope`, state.alternateLabel, undefined, true, async () => setEnablement(state.alternateState))) |
There was a problem hiding this comment.
AI Review: Every dropdown click creates a new caller-owned Action and retains it in the detail-wide renderDisposables until the detail rerenders or closes. ButtonWithDropdown disposes its menu view items, not these actions, so repeatedly opening the menu accumulates emitters and callbacks. Reuse one action per rendered button, return an unregistered lightweight action, or dispose actions when each menu closes.
| : localize('searchMcpPlaceholder', "Type to search...") | ||
| ); | ||
| try { | ||
| const pager = await this.mcpWorkbenchService.queryGallery(undefined, cts.token); |
There was a problem hiding this comment.
AI Review: This gallery request is still reachable when the effective chat.mcp.access value is none, including when policy-enforced. Selecting the MCP section can therefore issue a marketplace GET even though access-disabled UI is shown, and revocation leaves delayed or in-flight searches alive. Include effective access in snapshot/search eligibility and cancel delayedGallerySearch plus galleryCts when access becomes none; cover initial disabled visibility and revocation in tests.
| if (this._store.isDisposed || this.current !== item) { | ||
| return; | ||
| } | ||
| const installed = this.getInstalledPluginForMarketplaceItem(item); |
There was a problem hiding this comment.
AI Review: A successful install updates installed descriptors before MarketplaceAgentPluginDiscovery asynchronously publishes the plugin through IAgentPluginService.plugins, so this immediate lookup can miss. The detail does not observe the later publication and has no missing-result recovery, leaving the action disabled as Installing... until the view is recreated. Await or observe discovery of the expected URI, or return the installed plugin explicitly, and restore Install if registration never appears.
| switchElement.title = blocked ? localize('pluginPolicyBlockedSwitch', "This plugin is managed by your organization.") : toggleLabel; | ||
| DOM.append(switchElement, $('.plugin-enable-switch-thumb')); | ||
| this.cardDisposables.add(DOM.addDisposableListener(switchElement, 'click', () => { | ||
| const nextState = getToggledPluginEnablementState(current); |
There was a problem hiding this comment.
AI Review: This derives the transition from enablement captured when the row rendered, then the model update triggers an asynchronous full-card refresh. Before that refresh finishes, repeated activation writes the same transition while the live class and ARIA state remain stale; once it finishes, rebuilding cardContainer detaches the focused switch without restoring focus. Read current enablement per activation and update the row in place, or synchronously bind its state and restore the same row/action after rendering.
| uninstallButton.label = uninstallAction.label; | ||
| uninstallButton.enabled = uninstallAction.enabled; | ||
| this.renderDisposables.add(uninstallButton.onDidClick(async () => { | ||
| await uninstallAction.run(); |
There was a problem hiding this comment.
AI Review: The active remove contract is void, but CLI and extension implementations are async and await confirmation. UninstallPluginAction discards that runtime promise and resolves immediately, so this await navigates back while confirmation is pending, including when the user later cancels or removal fails. Return an awaited success result from removal, propagate it through the action, and navigate only after confirmed success.
| private setInstalledEntryEnabled(entry: IMcpInstalledEntry, enabled: boolean): void { | ||
| const activeSessionServer = getActiveSessionServer(entry); | ||
| if (activeSessionServer) { | ||
| activeSessionServer.setEnabled(enabled); |
There was a problem hiding this comment.
AI Review: For an installed row with an active-session counterpart, this unqualified Enable/Disable switch writes only a Session decision and leaves profile/workspace enablement unchanged. A new session can therefore show the server enabled again even though the switch appeared to disable it generally. Label this control as session-scoped or have the primary switch update the durable/default scope.
| const source = item.source; | ||
| const isWorkspaceFile = source === AICustomizationSources.local; | ||
| const isReadOnly = !source || source === AICustomizationSources.extension || source === AICustomizationSources.plugin || source === AICustomizationSources.builtin; | ||
| this.showEmbeddedEditor(item.uri, item.name, item.promptType, source ?? AICustomizationSources.builtin, isWorkspaceFile, isReadOnly); |
There was a problem hiding this comment.
AI Review: The two preceding section-load awaits are not tied to the initiating navigation. If the user selects another section or view while either is pending, this stale continuation can still open the old plugin contribution and replace the newer destination. Track a navigation generation or cancellation token and recheck the requested section/view before calling showEmbeddedEditor.
Summary
This PR substantially redesigns the AI Customizations management editor so it can act as the central place to discover, create, inspect, enable, disable, install, migrate, and manage the customizations available to the active agent harness.
The update covers both the regular VS Code workbench and the Agents window. It replaces the previous mostly list-oriented experience with a harness-aware overview, richer card and inventory surfaces, embedded detail pages, responsive layouts, and consistent navigation across agents, skills, instructions, hooks, prompts, MCP servers, plugins, models, and tools.
User experience changes
New overview and navigation
Agents, skills, instructions, hooks, and prompts
Customization migration
*.prompt.mdfiles into skills;MCP servers
Plugins
Tools
Responsive layout and presentation
Architecture and consistency
IAICustomizationItemsModelpipeline.Tests and fixtures
This PR expands focused coverage for:
The component fixture coverage now includes the overview and all major sections in light/dark themes, narrow layouts, browse/search states, migration flows, empty/disabled states, and embedded MCP/plugin detail pages.
Suggested manual verification
Screenshots