feat(mcp-store): marketplace with installed-server rail and per-tool approval#1747
feat(mcp-store): marketplace with installed-server rail and per-tool approval#1747
Conversation
|
This PR is dependent on a PR in the main /posthog repo: PostHog/posthog#55407 |
…proval test - Revert apps/code/src/renderer/App.tsx to main (unrelated invite-gate changes and the relative-import workaround were never part of the MCP refactor). - Delete filterInstallationsByCategory; the category cross-filter lives in the rail as a local query now, so the helper has no callers. - Extract dispatchBulkApproval into a testable pure helper and cover its parallel dispatch, failure propagation, and removed-tool skip.
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/code/src/renderer/features/settings/components/sections/McpServersSettings.tsx
Line: 131-142
Comment:
**Custom install doesn't navigate to the new installation**
After a successful custom server install the view is reset to `{ kind: "marketplace" }`, so the user never lands on the new installation's detail. The test plan explicitly requires: _"Adding a custom OAuth server…completes DCR + OAuth and lands on the new installation."_
The template flow can auto-advance because the `useEffect` at line 117-125 matches the returned `installationList` entry by `template_id`. Custom installations have no `template_id`, so that effect never fires.
To fix this, `installCustomMutation` in `useMcpServers` should return the new `McpServerInstallation` (the API already does — `installCustomMcpServer` resolves to `McpServerInstallation | OAuthRedirectResponse`, and after `runOAuthInstall` you re-fetch the list). One approach: have the `onSuccess` callback in `McpServersSettings` receive the installation ID from the invalidated list (or return it from the mutation) and call `setView({ kind: "detail-installation", installationId })` instead of reverting to marketplace.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/code/src/renderer/features/settings/hooks/mcpFilters.test.ts
Line: 73
Comment:
**`filterInstallationsByQuery` has no test coverage**
`mcpFilters.ts` exports three functions, but the test file only covers two of them — `filterInstallationsByQuery` is entirely untested. It is non-trivial: it searches across `display_name`, `name`, `url`, `description`, and template fields pulled from a `Map`. Given that the sibling functions each have five or more parameterised cases, and the test plan explicitly says "Rail search filters the installed list", this function deserves the same treatment.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "chore(mcp): revert stray App.tsx drift, ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (view.kind === "add-custom") { | ||
| return ( | ||
| <AddCustomServerForm | ||
| pending={installCustomPending} | ||
| onBack={() => setView({ kind: "marketplace" })} | ||
| onSubmit={(values) => { | ||
| installCustom(values, { | ||
| onSuccess: () => setView({ kind: "marketplace" }), | ||
| }); | ||
| }} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
After a successful custom server install the view is reset to { kind: "marketplace" }, so the user never lands on the new installation's detail. The test plan explicitly requires: "Adding a custom OAuth server…completes DCR + OAuth and lands on the new installation."
The template flow can auto-advance because the useEffect at line 117-125 matches the returned installationList entry by template_id. Custom installations have no template_id, so that effect never fires.
To fix this, installCustomMutation in useMcpServers should return the new McpServerInstallation (the API already does — installCustomMcpServer resolves to McpServerInstallation | OAuthRedirectResponse, and after runOAuthInstall you re-fetch the list). One approach: have the onSuccess callback in McpServersSettings receive the installation ID from the invalidated list (or return it from the mutation) and call setView({ kind: "detail-installation", installationId }) instead of reverting to marketplace.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/settings/components/sections/McpServersSettings.tsx
Line: 131-142
Comment:
**Custom install doesn't navigate to the new installation**
After a successful custom server install the view is reset to `{ kind: "marketplace" }`, so the user never lands on the new installation's detail. The test plan explicitly requires: _"Adding a custom OAuth server…completes DCR + OAuth and lands on the new installation."_
The template flow can auto-advance because the `useEffect` at line 117-125 matches the returned `installationList` entry by `template_id`. Custom installations have no `template_id`, so that effect never fires.
To fix this, `installCustomMutation` in `useMcpServers` should return the new `McpServerInstallation` (the API already does — `installCustomMcpServer` resolves to `McpServerInstallation | OAuthRedirectResponse`, and after `runOAuthInstall` you re-fetch the list). One approach: have the `onSuccess` callback in `McpServersSettings` receive the installation ID from the invalidated list (or return it from the mutation) and call `setView({ kind: "detail-installation", installationId })` instead of reverting to marketplace.
How can I resolve this? If you propose a fix, please make it concise.| it("returns empty when nothing matches", () => { | ||
| expect(filterServersByQuery(all, "zzz")).toEqual([]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
filterInstallationsByQuery has no test coverage
mcpFilters.ts exports three functions, but the test file only covers two of them — filterInstallationsByQuery is entirely untested. It is non-trivial: it searches across display_name, name, url, description, and template fields pulled from a Map. Given that the sibling functions each have five or more parameterised cases, and the test plan explicitly says "Rail search filters the installed list", this function deserves the same treatment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/settings/hooks/mcpFilters.test.ts
Line: 73
Comment:
**`filterInstallationsByQuery` has no test coverage**
`mcpFilters.ts` exports three functions, but the test file only covers two of them — `filterInstallationsByQuery` is entirely untested. It is non-trivial: it searches across `display_name`, `name`, `url`, `description`, and template fields pulled from a `Map`. Given that the sibling functions each have five or more parameterised cases, and the test plan explicitly says "Rail search filters the installed list", this function deserves the same treatment.
How can I resolve this? If you propose a fix, please make it concise.Adds a search input to the tools section of the MCP server detail screen that filters the tool list by name. The search field appears when there are more than 5 tools and includes a clear button.
cvolzer3
changed the title
1 participant
Summary
Rebuilds the MCP settings scene to match the new PostHog MCP store shape and the handoff prototype.
install_template/for marketplace connects,install_custom/for user-supplied URLs with optional client creds.-32001/-32002.fullwidthflag on Settings categories so MCP opts out of the standard 800px centered container; every other page is unchanged.auth_type: "none"branch in the main-process auth adapter — every installation routes through the MCP proxy.Test plan
needs_approval.pnpm --filter code typecheck,pnpm lint, andpnpm --filter code testall pass.