Skip to content

Three standalone MCP/customizations fixes: group-header casing, unclickable Show Output, duplicated server rows - #330890

Merged
Ulugbek Abdullaev (ulugbekna) merged 3 commits into
mainfrom
ulugbekna/mcp-standalone-fixes
Aug 17, 2026
Merged

Three standalone MCP/customizations fixes: group-header casing, unclickable Show Output, duplicated server rows#330890
Ulugbek Abdullaev (ulugbekna) merged 3 commits into
mainfrom
ulugbekna/mcp-standalone-fixes

Conversation

@ulugbekna

@ulugbekna Ulugbek Abdullaev (ulugbekna) commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Three unrelated MCP/customizations bug fixes, each still present on main today, kept as three separate commits so they stay individually revertable.

They were extracted from #330207, which has been overtaken by upstream and needs a rework against the new setCustomizationEnablement layer. These three do not touch enablement, so there is no reason to hold them behind that.

Important

6 screenshot baselines change, including two on the Instructions tab. That is expected, not scope creep: fix 1 is a one-line CSS change to the shared customizations group header, so it reaches every tab and both fixture families that render it. Each difference is only a text-casing correction with no layout shift. Full list in fix 1 below.


1. text-transform: capitalize mangles localized group headers

chat: fix: stop capitalizing localized customization group headers4bd2a2b

Symptom. Group headers re-cased strings that were already cased correctly, and got them wrong:

Localized string Rendered before
"Included Based on Context" Included Based On Context
"Loaded on Demand" Loaded On Demand
"Built-in" Built-In

Root cause. .ai-customization-group-header .group-label set text-transform: capitalize. The labels reaching it are already correctly-cased localized strings, so the transform could only ever re-case them — and per-word capitalization is not a transform that survives translation, so the rule was wrong for every locale rather than just awkward in English. The "On" cases also violate this repo's own UI-label rule that prepositions of four letters or fewer stay lowercase.

Why it is safe. The header is shared, so this reaches every customizations tab — Agents, Skills, Instructions, Hooks, Prompts, Plugins and MCP Servers — not just MCP. I checked every group label feeding it (aiCustomizationListWidget.ts, pluginListWidget.ts, mcpListWidget.ts): all are already correctly-cased localized strings ("Workspace", "User", "Plugins", "Extensions", "Built-in", "Remote", "Enabled Locally", "Disabled Locally", "Agent Instructions", …). So the only rendered differences are the three the transform was getting wrong.

Fixture impact — please accept these baselines. Rendering each affected fixture family on main's CSS and on this branch, 6 images differ:

Fixture Themes Difference
aiCustomizationManagementEditor/InstructionsTab Dark, Light Included Based On Context → Included Based on Context
aiCustomizationManagementEditor/McpServersTabScrolled Dark, Light Built-In → Built-in
aiCustomizationListWidget/InstructionsTabWithItems Dark, Light Included Based On Context → Included Based on Context

This matches the screenshot-diff report on the PR exactly. I originally listed only the first four: I had rendered just the aiCustomizationManagementEditor family, and the shared header is also used by aiCustomizationListWidget. Each of the six was then checked visually — only the casing changes, with no layout shift, no other text, and no other fixture affected. InstructionsTab moving in an MCP-titled PR is the shared header fix doing its job.


2. Show Output is unclickable on a failing MCP server

mcp: fix: make Show Output clickable on a failing MCP server — 624a7c3

Symptom. The inline Show Output button on an erroring MCP server row does nothing when clicked — on precisely the rows that need it, since a failing row is the only kind that offers the button at all.

Root cause. McpServerItemRenderer.updateStatus began with an unconditional

templateData.actionDisposables.clear();
DOM.clearNode(templateData.actions);

and then rebuilt the actions. It runs from an autorun over the server's connection state, and an erroring server re-runs it roughly twice a second while producing byte-identical content — measured in a real Code OSS build at 9–10 rebuilds per 5 seconds, every one a no-op. A DOM node replaced between mousedown and mouseup never receives the click, so the button was being destroyed out from under the user mid-press.

Fix. Rebuild only when something about the actions actually changed. getMcpStatusRenderSignature reduces a row's actions to a comparable value; updateStatus early-returns when it is unchanged.

The signature has to cover everything the actions are built from — both what they render (status label, class, icon, and the server name that goes into the button titles and aria labels) and what they act on (the active-session twin's id and log channel, the local server it falls back to, and the session resource captured when the output handler is built). Leaving any of it out would silently drop an update that matters, so it is a pure exported function rather than a comment. Its test uses a { [K in keyof IMcpStatusRenderInput]-?: … } mapped type, so adding a field to the input fails to compile until it is given a differing value.

The list re-splices on every customizations change, so renderElement would otherwise undo all of this by clearing the actions itself. It now keys on the row's content identity rather than the entry object — entries are recreated on every refresh, so object identity says nothing about whether this is still the same row.

Both guards are pinned by renderer-level tests, since that is the only place the failure is observable: an erroring server's update is fired repeatedly with unchanged content, and the button must be the same node afterwards, still attached, and still reach showMcpServerLog when clicked. A third test asserts a real status change does still rebuild, so the guard cannot be satisfied by never updating. Verified to bite by reverting each guard in turn — removing the updateStatus early return fails 2 of 3; defeating the renderElement row guard fails the re-render test.

Why it is safe. actionDisposables is only ever added to inside updateStatus, always after its own clear(), so nothing accumulates when a rebuild is skipped. The autoruns in renderElement still get re-created unconditionally, because they capture the fresh element. When the signature is unchanged, the retained buttons are bound to values the signature proves are equivalent.


3. One agent MCP server renders as two rows

mcp: fix: show one row per agent MCP server, not one per customizationde3185c

Symptom. A single MCP server shows up twice in the servers list, with contradictory status.

Root cause. A session can carry two customizations for one server. The agent host publishes the declaration as a child of whatever declared it (a plugin, or the .mcp.json VS Code syncs into the agent), and separately mints a top-level customization for any server the SDK reports before that child resolves by name. McpCustomizationController._applyOne never retires the minted entry once the child becomes resolvable — its own comment says "Once promoted to a top-level entry, stay top-level for the session" — so both persist. From a user's AHP logs:

notion -> file:///.../agentPlugins/vscode-synced-customization-.../.mcp.json#mcp=notion
          state: stopped                 <- the declaration
notion -> mcp-top-level:copilotcli:<session>:notion
          state: ready, channel: mcp://  <- the live one

getMcpServers returned both. It is worse than a repeat: the list's ActiveSessionMcpServerMatcher.take() only matches when exactly one candidate answers a key, so with two copies the server's local row cannot adopt either, and both fall through as extra rows.

Fix. getMcpServers drops a child that a top-level customization already speaks for. The top-level copy wins because it is the one the host treats as live — it carries the running state and channel, and its id is what the host resolves for lifecycle and enablement.

The signal is position in the tree, deliberately not two tempting alternatives:

  • not the shape of the minted id, which is the host's own business and not something the workbench should be parsing;
  • not "has no owning plugin". flattenMcpServerCustomizations sets plugin only for CustomizationType.Plugin containers, so a Directory-declared child carries plugin: undefined too. Treating that as top-level would let a directory child both claim its name and exempt itself from shadowing — i.e. re-create the bug for exactly the .mcp.json case above. There is a regression test for the directory container specifically.

Why it is safe. Nothing else is collapsed: two plugins that each declare a server named search stay two rows, because they are two servers. And only the presentation path dedupes — showMcpServerLog, _findMcpServer, and the diagnostics/tracking paths still walk every customization via flattenMcpServerCustomizations, so an id from either copy continues to resolve.

Follow-up (out of scope). The true repair is upstream in McpCustomizationController._applyOne (src/vs/platform/agentHost/node/shared/mcpCustomizationController.ts), which should retire the minted top-level entry once the declaring child becomes resolvable. This PR fixes the symptom in the workbench; the host-side repair is tracked in #330893.


Validation

  • npm run transpile-client — clean
  • npx tsc --project ./src/tsconfig.json --noEmit --skipLibCheck — no errors from this PR (the 2 remaining are present on clean main too, in agentHost/node/copilot and agentHost/test/node)
  • ./scripts/test.sh --grep "aiCustomization|mcpListWidget|agent host|agentHost|MCP" — 591 passing, 0 failing
  • npm run eslint <touched files> — clean
  • npm run stylelint — no new findings
  • npm run valid-layers-check — clean
  • npx component-explorer render … over the aiCustomizationManagementEditor and aiCustomizationListWidget families — 6 images differ, each verified as the fix 1 casing improvement, matching the screenshot-diff report

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes three AI customization/MCP UI regressions.

Changes:

  • Preserves localized group-header casing.
  • Avoids rebuilding unchanged MCP status actions.
  • Deduplicates agent-host MCP server rows and adds tests.
Show a summary per file
File Description
aiCustomizationManagement.css Removes automatic header capitalization.
mcpListWidget.ts Stabilizes inline status actions.
mcpListWidget.test.ts Tests status signatures.
agentHostCustomizationService.ts Deduplicates presentable MCP servers.
agentHostMcpServerCustomizations.test.ts Tests server deduplication.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

  • Files reviewed: 5/5 changed files
  • Comments generated: 4
  • Review effort level: Balanced

@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Screenshot Changes

Base: 3c7151d6 Current: 6d3fc24f

Changed (6)

chat/aiCustomizations/aiCustomizationListWidget/InstructionsTabWithItems/Dark
Before After
before after
chat/aiCustomizations/aiCustomizationListWidget/InstructionsTabWithItems/Light
Before After
before after
chat/aiCustomizations/aiCustomizationManagementEditor/InstructionsTab/Dark
Before After
before after
chat/aiCustomizations/aiCustomizationManagementEditor/InstructionsTab/Light
Before After
before after
chat/aiCustomizations/aiCustomizationManagementEditor/McpServersTabScrolled/Dark
Before After
before after
chat/aiCustomizations/aiCustomizationManagementEditor/McpServersTabScrolled/Light
Before After
before after

`text-transform: capitalize` on the shared group header re-cased strings that
were already cased correctly, and got them wrong: "Included Based on Context"
rendered as "Included Based On Context" and "Loaded on Demand" as "Loaded On
Demand", against this repo's own rule that short prepositions stay lowercase,
and "Built-in" rendered as "Built-In".

Per-word capitalization is also not a transform that survives translation, so
the rule was wrong for every locale rather than just awkward in English.

The header is shared, so this reaches every customizations tab -- Agents,
Skills, Instructions, Hooks, Prompts, Plugins and MCP Servers. Every group
label on those tabs is already a correctly cased localized string, so the only
rendered differences are the three the transform was getting wrong: four
screenshot baselines change, on the Instructions and MCP Servers tabs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
`updateStatus` began by clearing the row's action disposables and emptying its
actions node, then rebuilt them. It runs from an autorun over the server's
connection state, and an erroring server re-runs it about twice a second while
producing byte-identical content: measured in a real Code OSS build, 9-10
rebuilds per 5 seconds, every one of them a no-op.

A DOM node replaced between mousedown and mouseup never receives the click, so
the inline `Show Output` button did nothing on precisely the rows that needed
it -- the failing ones, which are the only rows that offer it at all.

The row's actions are now rebuilt only when something about them changed.
`getMcpStatusRenderSignature` reduces them to a comparable value covering both
what they render and what they act on; leaving anything out would drop an
update that matters, so it is a pure exported function whose test fails to
compile if a field is added without being covered.

The list re-splices on every customizations change, so `renderElement` would
otherwise undo this by clearing the actions itself. It now keys on the row's
content identity rather than the entry object, which is recreated on every
refresh and therefore says nothing about whether this is the same row.

Both guards are pinned by tests that drive the renderer itself, since that is
the only place the failure is observable: an erroring server's update is fired
repeatedly with unchanged content, and the button must be the same node
afterwards, still attached, and still reach `showMcpServerLog` when clicked. A
third test asserts a real status change does still rebuild, so the guard cannot
be satisfied by never updating. Reverting either guard fails them.
`McpServerItemRenderer` is exported for this.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A session can carry two customizations for a single MCP server. The agent host
publishes the declaration as a child of whatever declared it -- a plugin, or
the .mcp.json VS Code syncs into the agent -- and separately mints a top-level
customization for any server the SDK reports before that child can be resolved
by name. `McpCustomizationController._applyOne` never retires the minted entry
once the child becomes resolvable ("Once promoted to a top-level entry, stay
top-level for the session"), so both remain in state:

    notion -> file:///.../vscode-synced-customization-.../.mcp.json#mcp=notion
              state: stopped                 <- the declaration
    notion -> mcp-top-level:copilotcli:<session>:notion
              state: ready, channel: mcp://  <- the live one

Every consumer of getMcpServers saw both, so the servers list rendered the same
server twice with contradictory status. It was worse than a repeat: the list's
matcher only matches when exactly one candidate answers a key, so with two
copies the server's local row could not adopt either, and both fell through as
extra rows.

getMcpServers now drops a child that a top-level customization already speaks
for. The top-level copy wins because it is the one the host treats as live: it
carries the running state and channel, and its id is what the host resolves for
lifecycle and enablement. Position in the tree is the signal, not the shape of
the minted id, which is the host's own business -- and not the absence of an
owning plugin either, since a directory-declared child has none. That last one
is a live trap rather than a hypothetical, so it has its own test.

Nothing else is collapsed. Two plugins that each declare a server named
`search` stay two rows, because they are two servers. Only the presentation
path dedupes; log, diagnostics and id lookups still walk every customization,
so an id from either copy continues to resolve.

The host-side repair is tracked in #330893.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@ulugbekna
Ulugbek Abdullaev (ulugbekna) merged commit 0fff929 into main Aug 17, 2026
27 checks passed
@ulugbekna
Ulugbek Abdullaev (ulugbekna) deleted the ulugbekna/mcp-standalone-fixes branch August 17, 2026 17:13
@vs-code-engineering vs-code-engineering Bot added this to the 1.135.0 milestone Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants