feat: move MCP servers to AI settings - #26642
Conversation
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 4 | Last posted: Round 4, 17 findings (1 P1, 2 P2, 11 P3, 3 Nit), COMMENT. Review Finding inventoryFindings
Contested and acknowledgedCRF-2 (Nit, ProviderField.tsx:1) - Unrelated file deletion bundled with MCP migration
CRF-3 (P3, MCPServerFormFieldPrimitives.tsx:43) - CollapsibleSection duplicates existing component in ModelFormFields.tsx
CRF-4 (P3, MCPServersPageView.stories.tsx:1) - Story coverage still thin
Law analysisEffective LOC: +1979 -2105 (29 files). Head SHA: ac321a8. Verdict: Don't split. Enforcement: Advisory. Round logRound 1Netero-only. 1 P2, 1 Nit (Law). Reviewed against 7ea5d48..ac321a8. Round 2Panel. CRF-1 addressed. CRF-2 contested, panel closed (6/6 accept). 1 P1, 1 P2, 6 P3, 2 Nit new. Reviewed against 7ea5d48..922f8a9. Round 3Panel. CRF-5 (P1), CRF-6 (P2), CRF-7-15 verified fixed. CRF-3 closed (3/3), CRF-4 closed (4/4). 2 P3 new. Reviewed against 7ea5d48..65f923b. Round 4CRF-16, CRF-17 addressed, verified by Netero. 0 open findings. Clean. Reviewed against b11cd07..d7fbc05. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review (Netero). One P2, one Nit.
This is a first-pass review only: these are mechanical findings from Netero. The full review panel has not yet reviewed this PR. The panel will review after these findings are addressed.
The migration is structurally clean: no emdashes, no stale references, no dead exports, no circular imports, tsc passes, all 6 story tests pass. Error handling follows the toast pattern consistently. Law evaluated the LOC (1979 additions) and concluded "Don't split" (advisory) since the concerns form a single linear dependency chain (pages -> routes -> deletion) in a single frontend domain.
"This is 1742 new production lines with 134 test/fixture lines (13:1 ratio, project threshold is 5:1)." - Netero
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review (round 2). 1 P1, 1 P2, 6 P3, 2 Nit.
The migration is well-structured: routes mirror the Models page, the monolithic panel decomposition is clean, permission gating is consistent, error handling follows the toast pattern, and the MUI-to-design-system direction is correct. Four commits with a clear feature/test split.
The P1 is a data integrity issue where the header enable/disable toggle and form submit hold competing copies of enabled. Four independent reviewers found it. The P2 is an accidental credential wipe: focus a secret field, blur without typing, save.
CRF-2 (ProviderField deletion) is closed: 6/6 reviewers verified the file has zero remaining consumers.
"Two state owners, one field, no synchronization. The header toggle mutates the server directly. The form holds a stale copy of the same field." - Hisoka
Process note: commit messages on this 2100-line PR are sparse. test(site): cover MCP server settings feedback doesn't name the feedback or the tests. Consider adding commit bodies that reference the finding IDs or describe the change.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Re-review (round 3). 0 P0-P1 open. 2 new P3.
All 11 addressed findings verified fixed by the panel. The P1 (toggle/form enabled desync) is clean: buildUpdateMCPServerConfigRequest strips enabled, the toggle owns it exclusively, and unit test + story both verify. The P2 (secret focus-blur) is clean: onBlur restores the placeholder and resets touched, with defense-in-depth empty-string guards in the request builder.
CRF-3 (CollapsibleSection duplication) closed by panel (3/3). CRF-4 (story coverage) closed by panel (4/4). Both defenses hold for a migration PR.
Test density improved from 6.8% (R1) to 15.2% (R3). Production-to-test ratio is 5.57:1.
"The tests that exist are genuine gems. The Add story proves slug auto-generation, form gating, auth mode switching, and submit payload." - Bisky
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 17 findings resolved across 4 rounds. No open issues.
The P1 (toggle/form enabled desync, found by 4 reviewers) is fixed: buildUpdateMCPServerConfigRequest strips enabled, leaving the toggle as sole owner. Verified by unit test and story. The P2 (secret focus-blur wipe) is fixed: onBlur restores placeholder and resets touched, with defense-in-depth guards in the request builder.
Contest closures: CRF-2 (6/6), CRF-3 (3/3), CRF-4 (4/4), all with verified defenses.
Test density improved from 6.8% (R1) to 15.3% (R4). Production-to-test ratio is 4.4:1, under the 5:1 threshold. tsc --noEmit passes, all 7 unit tests pass, all story tests pass.
"I tried to build a case that the premises were wrong, that this change solves the wrong problem, or that the fix level was too shallow. I couldn't." - Pariston
🤖 This review was automatically generated with Coder Agents.
Documentation CheckUpdates Needed
Automated review via Coder Agents |
Docs preview📖 View docs preview for |
Fixed in |
DanielleMaywood
changed the title
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
Approving to unblock. This will be manually smoke-tested.
2 participants
Moves MCP server administration out of Coder Agents settings and into route-based AI Settings pages that follow the Models page structure.
This adds AI Settings list, add, and edit routes for MCP servers, redirects the old Agents settings route, updates both settings sidebars, and preserves the existing MCP form behavior for secrets, custom headers, availability, and tool governance.
Note
🤖 This PR was written by Coder Agent on behalf of Danielle Maywood
Implementation plan
MCP servers route-based AI Settings migration plan
Problem statement
MCP server settings currently live under Agents settings and use a monolithic query-param panel. The Models settings page was moved and redesigned under AI Settings. MCP servers should follow the same route-based AI Settings architecture and match the attached designs.
Approved direction
Use route-based AI Settings pages rather than keeping the existing
?server=panel flow.New routes:
/ai/settings/mcp-servers/ai/settings/mcp-servers/add/ai/settings/mcp-servers/:serverIdCompatibility route:
/agents/settings/mcp-serversredirects to/ai/settings/mcp-serversFiles likely to change
site/src/router.tsxsite/src/modules/management/AISettingsSidebarView.tsxsite/src/modules/dashboard/Navbar/Navbar.tsxsite/src/pages/AgentsPage/components/ChatsSidebar/settings/SettingsPanel.tsxsite/src/pages/AISettingsPage/MCPServersPage/Implementation steps
/agents/settings/mcp-serversto/ai/settings/mcp-servers.permissions.editDeploymentConfig./ai/settings/mcp-serverswith external-link treatment, matching Providers and Models.MCPServersPagecontainer that uses existing React Query MCP helpers andRequirePermission.MCPServersPageViewwithSettingsHeader, shared table primitives, loading, error, empty, and populated states.MCPServerRowwith clickable row behavior, icon, auth method, availability, status badge, and chevron.MCPServerForm.Rejected alternatives
Reuse
MCPServerAdminPanelunchangedRejected because it preserves the old Agents visual style and query-param navigation, which conflicts with the Models page and Figma direction.
Light restyle of the existing monolith
Rejected because the component mixes list, URL state, form logic, dialogs, and field rendering. Splitting route-based pages is more maintainable and closer to the Models page.
Edge cases to preserve
Verification plan
Targeted checks:
cd site pnpm format pnpm lint pnpm test:storybook src/pages/AISettingsPage/MCPServersPageManual or Storybook states to inspect:
Open questions
None.