feat(mcp): add a registered option to local mcps#2145
Open
bmahabirbu wants to merge 5 commits into
Open
Conversation
bmahabirbu
requested review from
gastoner and
jeffmaury
and removed request for
a team
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds support for registering MCP servers without immediately spawning them, introducing a 'registered' state distinct from 'running'. The implementation spans the data layer (status field), manager operations (client-less registration), registry logic (deferred initialization), IPC wiring, form workflows, and UI updates for status display and registration flows. ChangesMCP Server Registration Without Spawning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
registered local mcps allow the use of passing the config to the workspace without needing to have the mcp running locally on your host which saves us from blocking users from using mcps if they dont have pypi or npm installed spawned mcps were super useful when we had the chat but now it seems we are moving to mcps being run inside the vm/container so maybe less use cases for spawned mcps except for some additional tool info |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte`:
- Around line 25-26: The UI allows Starting "spawn" and "register" concurrently
because only spawning and registering track their own progress; add a shared
guard to block the alternate action while one request is in flight: either
introduce a single boolean (e.g., inFlight) or check the opposite flag at the
start of each handler (spawn handler and register handler) and return early if
the other is true, set the shared guard (or both flags) to true when a request
starts and reset it in the finally block so the other button is disabled during
the request; update the button bindings to use this shared guard so clicking one
action while the other is running is prevented (referencing the spawning and
registering flags and the spawn/register handlers used in the component).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ad2f5d8-9e4a-4762-a2c4-a7de6a8a0e1a
📒 Files selected for processing (10)
packages/api/src/mcp/mcp-server-info.tspackages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.tspackages/preload/src/index.tspackages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.sveltepackages/renderer/src/lib/mcp/MCPServerListRemoteReady.sveltepackages/renderer/src/lib/mcp/column/MCPStatusColumn.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: macOS
- GitHub Check: Windows
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: linter, formatters
- GitHub Check: unit tests / windows-2022
- GitHub Check: unit tests / macos-15
- GitHub Check: Linux
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
packages/api/src/mcp/mcp-server-info.tspackages/main/src/plugin/index.tspackages/preload/src/index.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
packages/main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/main/src/**/*.{ts,tsx}: UseipcHandle()to expose handlers in the main process with naming convention<registry-name>:<action>(e.g.,container-provider-registry:listContainers)
UseapiSender.send()to send events from main process to renderer for real-time updates
Long-running operations should useTaskManager.createTask()with title and action configuration
Files:
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
packages/main/src/plugin/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
All major services must be registered as singletons in the Inversify DI container during initialization in the PluginSystem
Files:
packages/main/src/plugin/index.ts
packages/{main,renderer,preload}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Container operations must include
engineIdparameter to identify the container engine
Files:
packages/main/src/plugin/index.tspackages/preload/src/index.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
🧠 Learnings (7)
📚 Learning: 2026-04-15T08:04:32.031Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1336
File: packages/renderer/src/lib/guided-setup/GuidedSetup.svelte:9-11
Timestamp: 2026-04-15T08:04:32.031Z
Learning: For Svelte components in this repo, if a callback prop is typed as `() => void`, TypeScript idiomatically allows passing async functions (e.g., `() => Promise<void>`), because `() => void` indicates the caller ignores the return value rather than requiring `undefined`. Do not recommend changing these prop types to `() => void | Promise<void>` solely to “fix” async compatibility—unless there is an actual need for the caller to observe the returned value.
Applied to files:
packages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/column/MCPStatusColumn.sveltepackages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.sveltepackages/renderer/src/lib/mcp/MCPServerListRemoteReady.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte
📚 Learning: 2026-04-28T13:34:51.610Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1431
File: packages/renderer/src/lib/guided-setup/panels/OpenCodePanel.svelte:14-20
Timestamp: 2026-04-28T13:34:51.610Z
Learning: In this repo’s Svelte renderer (packages/renderer/src/**/*.svelte), `podman-desktop/ui-svelte`’s `Link` component does not accept an `href` prop. For opening external URLs, use the established pattern `on:click={() => window.openExternal(url)}` (optionally typed as `on:click={(): Promise<void> => window.openExternal(url)}`), consistent with existing components like `ProviderLinks.svelte`, `WelcomePage.svelte`, and `OpenCodePanel.svelte`. Do not treat `window.openExternal()` usage in renderer Svelte components as bypassing a security restriction, and do not recommend replacing it with `Link`/`href`-based navigation. (There is also no `setupSecurityRestrictionsOnLinks` utility in the renderer.)
Applied to files:
packages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/column/MCPStatusColumn.sveltepackages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.sveltepackages/renderer/src/lib/mcp/MCPServerListRemoteReady.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte
📚 Learning: 2026-04-29T11:54:22.423Z
Learnt from: vancura
Repo: openkaiden/kaiden PR: 1494
File: packages/renderer/src/Loader.svelte:13-13
Timestamp: 2026-04-29T11:54:22.423Z
Learning: In this codebase’s renderer, timer variables in Svelte files (e.g., in `packages/renderer/src/Loader.svelte`) that store `setTimeout` handles are intentionally typed as `NodeJS.Timeout` (and not `ReturnType<typeof setTimeout>`). When reviewing, do not flag or recommend changing this to `ReturnType<typeof setTimeout>` for portability—treat `NodeJS.Timeout` as the established convention for renderer timer variables.
Applied to files:
packages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/column/MCPStatusColumn.sveltepackages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.sveltepackages/renderer/src/lib/mcp/MCPServerListRemoteReady.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
packages/renderer/src/lib/mcp/MCPDetails.sveltepackages/api/src/mcp/mcp-server-info.tspackages/main/src/plugin/index.tspackages/preload/src/index.tspackages/renderer/src/lib/mcp/column/MCPStatusColumn.sveltepackages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.sveltepackages/main/src/plugin/mcp/mcp-manager.tspackages/renderer/src/lib/mcp/MCPServerListRemoteReady.sveltepackages/main/src/plugin/mcp/mcp-registry.tspackages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte
📚 Learning: 2026-06-10T15:22:39.639Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 2115
File: packages/renderer/src/lib/models/SemanticRouterCreate.svelte:134-137
Timestamp: 2026-06-10T15:22:39.639Z
Learning: In multi-step wizard UIs, it’s acceptable for “Next step” buttons to be temporarily non-interactive during incremental work delivered across multiple PRs. When reviewing Svelte files under `packages/renderer/src/lib/**`, do not flag these UX issues if the button lacks an onClick handler *and* there is an explicit TODO comment indicating deferred wiring, and the PR author confirms (in the PR description) that this is an intentional staged implementation. If no TODO/deferred-wiring marker (or no author confirmation) is present, treat missing handlers as a potential UX issue.
Applied to files:
packages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/column/MCPStatusColumn.sveltepackages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.sveltepackages/renderer/src/lib/mcp/MCPServerListRemoteReady.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte
📚 Learning: 2026-03-09T08:47:09.657Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 1077
File: packages/main/src/plugin/skill/skill-manager.ts:80-109
Timestamp: 2026-03-09T08:47:09.657Z
Learning: In the kortex-hub/kortex repository, IPC handlers (via ipcHandle()) may be registered directly inside feature manager/service classes (e.g., SkillManager in packages/main/src/plugin/skill/skill-manager.ts) rather than exclusively in packages/main/src/plugin/index.ts. Treat this as an accepted design pattern for files under the plugin directory. Reviewers should not require centralization in index.ts; allow IPC registration proximity to the feature that owns the handler. When reviewing code, accept direct ipcHandle() registrations inside feature managers and ensure the pattern is consistently applied across similar feature-manager modules.
Applied to files:
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
📚 Learning: 2026-03-26T08:09:22.450Z
Learnt from: jeffmaury
Repo: kortex-hub/kortex PR: 1169
File: packages/main/src/plugin/mcp/mcp-registry.ts:135-151
Timestamp: 2026-03-26T08:09:22.450Z
Learning: In kortex-hub/kortex (main MCP plugin code), do not rely on `onDidUnregisterRagConnection` for MCP cleanup when tearing down a RAG connection. Per the RAG lifecycle contract, a stop update is always emitted before deletion/unregistration (i.e., `onDidUpdateRagConnection` with status `'stopped'`). Therefore, the `'stopped'` stop handler is sufficient for MCP cleanup; only add `onDidUnregisterRagConnection` if you find a concrete case where unregister can occur without a prior `'stopped'` update.
Applied to files:
packages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
🔇 Additional comments (9)
packages/api/src/mcp/mcp-server-info.ts (1)
37-37: LGTM!packages/main/src/plugin/mcp/mcp-registry.ts (1)
40-40: LGTM!Also applies to: 84-84, 277-304, 459-465, 503-503, 573-654
packages/main/src/plugin/mcp/mcp-manager.ts (1)
177-205: LGTM!Also applies to: 225-229
packages/preload/src/index.ts (1)
124-124: LGTM!Also applies to: 2353-2358
packages/renderer/src/lib/mcp/MCPServerListRemoteReady.svelte (1)
3-6: LGTM!Also applies to: 36-60
packages/main/src/plugin/index.ts (1)
145-145: LGTM!Also applies to: 2404-2409
packages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.svelte (1)
11-11: LGTM!Also applies to: 50-61, 102-102
packages/renderer/src/lib/mcp/column/MCPStatusColumn.svelte (1)
1-26: LGTM!packages/renderer/src/lib/mcp/MCPDetails.svelte (1)
34-45: LGTM!
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
jeffmaury
reviewed
Jun 12, 2026
jeffmaury
left a comment
jeffmaury
left a comment
Contributor
There was a problem hiding this comment.
Looks good but how can we start it once it's registered ?
bmahabirbu
force-pushed
the
mcp-spawn-fix
branch
from
e06c23f to
983f024
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/main/src/plugin/mcp/mcp-registry.ts`:
- Around line 648-654: The deletePackageFromConfiguration method exists but is
not being called from the package removal flow. Locate the IPC handler or delete
method that currently only removes remote configs and ensure it also calls
deletePackageFromConfiguration with the appropriate serverId and packageId
parameters when a package is being removed. This ensures package registrations
are properly deleted from storage and do not reappear after restart. Consider
whether to unify the remote config and package deletion logic into a single
method to avoid duplication.
- Around line 460-465: The duplicate prevention check at the pre-check stage
(around the existing server lookup) is insufficient for concurrent requests
since both calls can pass before either completes registration. Add a key-level
uniqueness guard at the actual registration point in the manager's registration
methods (where the server is actually being added to the registry) rather than
relying only on the pre-check. This guard should fail fast when attempting to
register a server with a serverId that already exists in the registry, ensuring
that only one registration succeeds even when multiple concurrent requests
bypass the initial pre-check. Apply this same fix to both the registration site
at line 460 and the similar site at line 573-579.
In `@packages/renderer/src/lib/mcp/MCPDetails.svelte`:
- Around line 34-45: The isRunning derived value currently evaluates to true
when mcpServer is undefined because undefined !== 'registered' is true, causing
refreshMessages() to be called prematurely before the server status is known.
Update the isRunning derived variable to add an explicit check that mcpServer is
defined before checking its status, so that undefined mcpServer is treated as
not-running, preventing refreshMessages() from being triggered in the onMount
hook and the mcp-manager-update event listener until the server state is
actually resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73992c9f-9bc2-44ab-b905-a5e7540ccfb1
📒 Files selected for processing (10)
packages/api/src/mcp/mcp-server-info.tspackages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.tspackages/preload/src/index.tspackages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.sveltepackages/renderer/src/lib/mcp/MCPServerListRemoteReady.sveltepackages/renderer/src/lib/mcp/column/MCPStatusColumn.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / windows-2022
- GitHub Check: Windows
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: Linux
- GitHub Check: macOS
- GitHub Check: typecheck
- GitHub Check: linter, formatters
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
packages/api/src/mcp/mcp-server-info.tspackages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/preload/src/index.tspackages/main/src/plugin/mcp/mcp-registry.ts
packages/main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/main/src/**/*.{ts,tsx}: UseipcHandle()to expose handlers in the main process with naming convention<registry-name>:<action>(e.g.,container-provider-registry:listContainers)
UseapiSender.send()to send events from main process to renderer for real-time updates
Long-running operations should useTaskManager.createTask()with title and action configuration
Files:
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
packages/main/src/plugin/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
All major services must be registered as singletons in the Inversify DI container during initialization in the PluginSystem
Files:
packages/main/src/plugin/index.ts
packages/{main,renderer,preload}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Container operations must include
engineIdparameter to identify the container engine
Files:
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/preload/src/index.tspackages/main/src/plugin/mcp/mcp-registry.ts
🧠 Learnings (7)
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
packages/api/src/mcp/mcp-server-info.tspackages/renderer/src/lib/mcp/MCPServerListRemoteReady.sveltepackages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.sveltepackages/renderer/src/lib/mcp/column/MCPStatusColumn.sveltepackages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/preload/src/index.tspackages/main/src/plugin/mcp/mcp-registry.ts
📚 Learning: 2026-04-15T08:04:32.031Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1336
File: packages/renderer/src/lib/guided-setup/GuidedSetup.svelte:9-11
Timestamp: 2026-04-15T08:04:32.031Z
Learning: For Svelte components in this repo, if a callback prop is typed as `() => void`, TypeScript idiomatically allows passing async functions (e.g., `() => Promise<void>`), because `() => void` indicates the caller ignores the return value rather than requiring `undefined`. Do not recommend changing these prop types to `() => void | Promise<void>` solely to “fix” async compatibility—unless there is an actual need for the caller to observe the returned value.
Applied to files:
packages/renderer/src/lib/mcp/MCPServerListRemoteReady.sveltepackages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.sveltepackages/renderer/src/lib/mcp/column/MCPStatusColumn.svelte
📚 Learning: 2026-04-28T13:34:51.610Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1431
File: packages/renderer/src/lib/guided-setup/panels/OpenCodePanel.svelte:14-20
Timestamp: 2026-04-28T13:34:51.610Z
Learning: In this repo’s Svelte renderer (packages/renderer/src/**/*.svelte), `podman-desktop/ui-svelte`’s `Link` component does not accept an `href` prop. For opening external URLs, use the established pattern `on:click={() => window.openExternal(url)}` (optionally typed as `on:click={(): Promise<void> => window.openExternal(url)}`), consistent with existing components like `ProviderLinks.svelte`, `WelcomePage.svelte`, and `OpenCodePanel.svelte`. Do not treat `window.openExternal()` usage in renderer Svelte components as bypassing a security restriction, and do not recommend replacing it with `Link`/`href`-based navigation. (There is also no `setupSecurityRestrictionsOnLinks` utility in the renderer.)
Applied to files:
packages/renderer/src/lib/mcp/MCPServerListRemoteReady.sveltepackages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.sveltepackages/renderer/src/lib/mcp/column/MCPStatusColumn.svelte
📚 Learning: 2026-04-29T11:54:22.423Z
Learnt from: vancura
Repo: openkaiden/kaiden PR: 1494
File: packages/renderer/src/Loader.svelte:13-13
Timestamp: 2026-04-29T11:54:22.423Z
Learning: In this codebase’s renderer, timer variables in Svelte files (e.g., in `packages/renderer/src/Loader.svelte`) that store `setTimeout` handles are intentionally typed as `NodeJS.Timeout` (and not `ReturnType<typeof setTimeout>`). When reviewing, do not flag or recommend changing this to `ReturnType<typeof setTimeout>` for portability—treat `NodeJS.Timeout` as the established convention for renderer timer variables.
Applied to files:
packages/renderer/src/lib/mcp/MCPServerListRemoteReady.sveltepackages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.sveltepackages/renderer/src/lib/mcp/column/MCPStatusColumn.svelte
📚 Learning: 2026-06-10T15:22:39.639Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 2115
File: packages/renderer/src/lib/models/SemanticRouterCreate.svelte:134-137
Timestamp: 2026-06-10T15:22:39.639Z
Learning: In multi-step wizard UIs, it’s acceptable for “Next step” buttons to be temporarily non-interactive during incremental work delivered across multiple PRs. When reviewing Svelte files under `packages/renderer/src/lib/**`, do not flag these UX issues if the button lacks an onClick handler *and* there is an explicit TODO comment indicating deferred wiring, and the PR author confirms (in the PR description) that this is an intentional staged implementation. If no TODO/deferred-wiring marker (or no author confirmation) is present, treat missing handlers as a potential UX issue.
Applied to files:
packages/renderer/src/lib/mcp/MCPServerListRemoteReady.sveltepackages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.sveltepackages/renderer/src/lib/mcp/column/MCPStatusColumn.svelte
📚 Learning: 2026-03-09T08:47:09.657Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 1077
File: packages/main/src/plugin/skill/skill-manager.ts:80-109
Timestamp: 2026-03-09T08:47:09.657Z
Learning: In the kortex-hub/kortex repository, IPC handlers (via ipcHandle()) may be registered directly inside feature manager/service classes (e.g., SkillManager in packages/main/src/plugin/skill/skill-manager.ts) rather than exclusively in packages/main/src/plugin/index.ts. Treat this as an accepted design pattern for files under the plugin directory. Reviewers should not require centralization in index.ts; allow IPC registration proximity to the feature that owns the handler. When reviewing code, accept direct ipcHandle() registrations inside feature managers and ensure the pattern is consistently applied across similar feature-manager modules.
Applied to files:
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
📚 Learning: 2026-03-26T08:09:22.450Z
Learnt from: jeffmaury
Repo: kortex-hub/kortex PR: 1169
File: packages/main/src/plugin/mcp/mcp-registry.ts:135-151
Timestamp: 2026-03-26T08:09:22.450Z
Learning: In kortex-hub/kortex (main MCP plugin code), do not rely on `onDidUnregisterRagConnection` for MCP cleanup when tearing down a RAG connection. Per the RAG lifecycle contract, a stop update is always emitted before deletion/unregistration (i.e., `onDidUpdateRagConnection` with status `'stopped'`). Therefore, the `'stopped'` stop handler is sufficient for MCP cleanup; only add `onDidUnregisterRagConnection` if you find a concrete case where unregister can occur without a prior `'stopped'` update.
Applied to files:
packages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
🔇 Additional comments (10)
packages/api/src/mcp/mcp-server-info.ts (1)
37-37: LGTM!packages/main/src/plugin/mcp/mcp-registry.ts (1)
40-40: LGTM!Also applies to: 84-84, 278-304, 503-503
packages/main/src/plugin/mcp/mcp-manager.ts (1)
177-205: LGTM!Also applies to: 225-229
packages/main/src/plugin/index.ts (1)
145-145: LGTM!Also applies to: 2404-2409
packages/preload/src/index.ts (1)
124-124: LGTM!Also applies to: 2353-2358
packages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte (2)
25-26: Concurrent spawn/register submission is still unguarded (already reported).This is the same unresolved finding previously raised for Line 25-Line 26 and the action handlers/buttons in this file.
Also applies to: 88-104, 178-190
2-23: LGTM!Also applies to: 78-86
packages/renderer/src/lib/mcp/MCPRegistryCreateFromRegistryForm.svelte (1)
11-11: LGTM!Also applies to: 50-61, 102-102
packages/renderer/src/lib/mcp/column/MCPStatusColumn.svelte (1)
1-26: LGTM!packages/renderer/src/lib/mcp/MCPServerListRemoteReady.svelte (1)
3-3: LGTM!Also applies to: 6-6, 37-39, 47-52, 60-60
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/main/src/plugin/mcp/mcp-manager.ts`:
- Around line 236-242: The addClient method overwrites an existing client stored
in `#client` without first checking if an entry for that key already exists, which
can leak resources and orphan the exchange chain. Before calling
this.#client.set(key, client), add a guard check that throws an error (or
appropriate exception) if `#client` already has an entry for the provided key.
This prevents duplicate starts from silently overwriting the prior client and
ensures fail-fast behavior for concurrent or duplicate start attempts.
In `@packages/main/src/plugin/mcp/mcp-registry.ts`:
- Around line 682-684: The issue is that runtime state mutation via
mcpManager.addClient() is performed before persisting the autoSpawn
configuration via updateConfigurationAutoSpawn(), creating a window where if
persistence fails, the runtime state and saved config diverge causing incorrect
restart behavior. Fix this by implementing transactional handling: either
persist the autoSpawn configuration first before calling mcpManager.addClient()
(persist-first approach), or implement rollback logic where if
updateConfigurationAutoSpawn() fails, you revert the runtime state change by
removing the client from mcpManager to restore consistency.
In `@packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts`:
- Around line 46-48: The beforeEach hook is missing fake timers configuration
and window API mocks needed by the tests. Add vi.useFakeTimers({
shouldAdvanceTime: true }) to configure Svelte component test timing correctly,
and mock the window.startMcpServer and window.stopMcpServer functions that are
asserted in the test cases (which are needed at the assertion points). This
ensures tests don't have timing flakiness and the window mocks are available
when the component under test calls these functions.
In `@packages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte`:
- Around line 25-36: The handleStartStop function lacks error handling for
failures in window.startMcpServer or window.stopMcpServer calls. Add a catch
block to the existing try/finally structure in handleStartStop to capture any
errors thrown during the start or stop operations, and provide user feedback
(following the error handling pattern used in the exportToTarget function) to
inform the user of any failures that occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48cb7fb7-1d00-489f-97df-691db68adacf
📒 Files selected for processing (8)
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.spec.tspackages/main/src/plugin/mcp/mcp-registry.tspackages/preload/src/index.tspackages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.tspackages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: unit tests / windows-2022
- GitHub Check: linter, formatters
- GitHub Check: unit tests / macos-15
- GitHub Check: typecheck
- GitHub Check: macOS
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.tspackages/main/src/plugin/index.tspackages/preload/src/index.tspackages/main/src/plugin/mcp/mcp-registry.spec.tspackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
**/*.spec.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.{ts,tsx,js,jsx}: Usetest()instead ofit()for test cases in Vitest unit tests
Usevi.mock(import('...'))for auto-mocking modules in unit tests; avoid manual mock factories when possible
Usevi.resetAllMocks()inbeforeEachhooks instead ofvi.clearAllMocks()for resetting mocks between tests
When an auto-mocked function or class method needs a real implementation, usevi.mocked(...)with the prototype pattern for class methods:vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)
Files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.tspackages/main/src/plugin/mcp/mcp-registry.spec.tspackages/main/src/plugin/mcp/mcp-manager.spec.ts
packages/{main,renderer,preload}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Container operations must include
engineIdparameter to identify the container engine
Files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.tspackages/main/src/plugin/index.tspackages/preload/src/index.tspackages/main/src/plugin/mcp/mcp-registry.spec.tspackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
packages/main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/main/src/**/*.{ts,tsx}: UseipcHandle()to expose handlers in the main process with naming convention<registry-name>:<action>(e.g.,container-provider-registry:listContainers)
UseapiSender.send()to send events from main process to renderer for real-time updates
Long-running operations should useTaskManager.createTask()with title and action configuration
Files:
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-registry.spec.tspackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
packages/main/src/plugin/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
All major services must be registered as singletons in the Inversify DI container during initialization in the PluginSystem
Files:
packages/main/src/plugin/index.ts
🧠 Learnings (8)
📚 Learning: 2026-04-15T08:51:08.199Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1336
File: packages/renderer/src/lib/guided-setup/GuidedSetup.spec.ts:64-69
Timestamp: 2026-04-15T08:51:08.199Z
Learning: For Svelte component test files under `packages/renderer/src/**` with names ending in `.spec.ts` that use `render` from `testing-library/svelte`, call `vi.useFakeTimers({ shouldAdvanceTime: true })` inside a `beforeEach`. Keep it in `beforeEach` for these tests to avoid flakiness from Svelte’s internal tick scheduling; do not remove or flag it based on the presence/absence of explicit timer manipulation in individual tests.
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.tspackages/main/src/plugin/index.tspackages/preload/src/index.tspackages/main/src/plugin/mcp/mcp-registry.spec.tspackages/renderer/src/lib/mcp/MCPServerRemoteListActions.sveltepackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
📚 Learning: 2026-03-09T08:47:09.657Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 1077
File: packages/main/src/plugin/skill/skill-manager.ts:80-109
Timestamp: 2026-03-09T08:47:09.657Z
Learning: In the kortex-hub/kortex repository, IPC handlers (via ipcHandle()) may be registered directly inside feature manager/service classes (e.g., SkillManager in packages/main/src/plugin/skill/skill-manager.ts) rather than exclusively in packages/main/src/plugin/index.ts. Treat this as an accepted design pattern for files under the plugin directory. Reviewers should not require centralization in index.ts; allow IPC registration proximity to the feature that owns the handler. When reviewing code, accept direct ipcHandle() registrations inside feature managers and ensure the pattern is consistently applied across similar feature-manager modules.
Applied to files:
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-registry.spec.tspackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
📚 Learning: 2026-03-26T08:09:22.450Z
Learnt from: jeffmaury
Repo: kortex-hub/kortex PR: 1169
File: packages/main/src/plugin/mcp/mcp-registry.ts:135-151
Timestamp: 2026-03-26T08:09:22.450Z
Learning: In kortex-hub/kortex (main MCP plugin code), do not rely on `onDidUnregisterRagConnection` for MCP cleanup when tearing down a RAG connection. Per the RAG lifecycle contract, a stop update is always emitted before deletion/unregistration (i.e., `onDidUpdateRagConnection` with status `'stopped'`). Therefore, the `'stopped'` stop handler is sufficient for MCP cleanup; only add `onDidUnregisterRagConnection` if you find a concrete case where unregister can occur without a prior `'stopped'` update.
Applied to files:
packages/main/src/plugin/mcp/mcp-registry.spec.tspackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.ts
📚 Learning: 2026-04-15T08:04:32.031Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1336
File: packages/renderer/src/lib/guided-setup/GuidedSetup.svelte:9-11
Timestamp: 2026-04-15T08:04:32.031Z
Learning: For Svelte components in this repo, if a callback prop is typed as `() => void`, TypeScript idiomatically allows passing async functions (e.g., `() => Promise<void>`), because `() => void` indicates the caller ignores the return value rather than requiring `undefined`. Do not recommend changing these prop types to `() => void | Promise<void>` solely to “fix” async compatibility—unless there is an actual need for the caller to observe the returned value.
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte
📚 Learning: 2026-04-28T13:34:51.610Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1431
File: packages/renderer/src/lib/guided-setup/panels/OpenCodePanel.svelte:14-20
Timestamp: 2026-04-28T13:34:51.610Z
Learning: In this repo’s Svelte renderer (packages/renderer/src/**/*.svelte), `podman-desktop/ui-svelte`’s `Link` component does not accept an `href` prop. For opening external URLs, use the established pattern `on:click={() => window.openExternal(url)}` (optionally typed as `on:click={(): Promise<void> => window.openExternal(url)}`), consistent with existing components like `ProviderLinks.svelte`, `WelcomePage.svelte`, and `OpenCodePanel.svelte`. Do not treat `window.openExternal()` usage in renderer Svelte components as bypassing a security restriction, and do not recommend replacing it with `Link`/`href`-based navigation. (There is also no `setupSecurityRestrictionsOnLinks` utility in the renderer.)
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte
📚 Learning: 2026-04-29T11:54:22.423Z
Learnt from: vancura
Repo: openkaiden/kaiden PR: 1494
File: packages/renderer/src/Loader.svelte:13-13
Timestamp: 2026-04-29T11:54:22.423Z
Learning: In this codebase’s renderer, timer variables in Svelte files (e.g., in `packages/renderer/src/Loader.svelte`) that store `setTimeout` handles are intentionally typed as `NodeJS.Timeout` (and not `ReturnType<typeof setTimeout>`). When reviewing, do not flag or recommend changing this to `ReturnType<typeof setTimeout>` for portability—treat `NodeJS.Timeout` as the established convention for renderer timer variables.
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte
📚 Learning: 2026-06-10T15:22:39.639Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 2115
File: packages/renderer/src/lib/models/SemanticRouterCreate.svelte:134-137
Timestamp: 2026-06-10T15:22:39.639Z
Learning: In multi-step wizard UIs, it’s acceptable for “Next step” buttons to be temporarily non-interactive during incremental work delivered across multiple PRs. When reviewing Svelte files under `packages/renderer/src/lib/**`, do not flag these UX issues if the button lacks an onClick handler *and* there is an explicit TODO comment indicating deferred wiring, and the PR author confirms (in the PR description) that this is an intentional staged implementation. If no TODO/deferred-wiring marker (or no author confirmation) is present, treat missing handlers as a potential UX issue.
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte
🔇 Additional comments (18)
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte (4)
3-3: LGTM!
20-20: LGTM!
22-23: LGTM!
98-104: LGTM!packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts (8)
19-27: LGTM!
28-44: LGTM!
50-54: LGTM!
56-60: LGTM!
62-67: LGTM!
69-76: LGTM!
78-85: LGTM!
87-91: LGTM!packages/main/src/plugin/mcp/mcp-manager.spec.ts (1)
27-116: LGTM!packages/main/src/plugin/mcp/mcp-registry.spec.ts (1)
35-613: LGTM!packages/main/src/plugin/index.ts (1)
2444-2450: LGTM!packages/preload/src/index.ts (3)
124-124: LGTM!
2353-2358: LGTM!
2393-2399: IPC channel handlers correctly registered; no functional issue.Verification confirms that the main process handlers for
'mcp-manager:startMCPServer'and'mcp-manager:stopMCPServer'are properly registered at packages/main/src/plugin/index.ts (lines 2444 and 2448), matching the IPC channels invoked from the preload layer. The IPC bridge will function correctly at runtime.Note: There is a minor casing inconsistency where preload function names use lowercase
Mcp(e.g.,startMcpServer) while the IPC channels use uppercaseMCP(e.g.,startMCPServer). This differs from other MCP functions in the codebase (e.g.,fetchMcpRemoteServersmaintains consistent casing). If desired, align the casing for consistency, though this is not required for functionality.
bmahabirbu
changed the title
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts (1)
46-48:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd fake timers and window API mocks to the
beforeEachhook.This is the same issue flagged in the previous review, with one addition: the new error handling test (lines 87-104) also requires
window.showMessageBoxto be mocked.Issues:
- Missing
vi.useFakeTimers({ shouldAdvanceTime: true })- required per learnings for Svelte component tests usingrender.- Missing mocks for
window.startMcpServer,window.stopMcpServer(asserted at lines 75, 84, 88).- Missing mock for
window.showMessageBox(asserted at line 96 in the new error test).🔧 Updated fix to include all required mocks
beforeEach(() => { + vi.useFakeTimers({ shouldAdvanceTime: true }); vi.resetAllMocks(); + window.startMcpServer = vi.fn(); + window.stopMcpServer = vi.fn(); + window.showMessageBox = vi.fn(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts` around lines 46 - 48, The beforeEach hook is missing fake timers configuration and window API mocks required for the test suite. Add vi.useFakeTimers({ shouldAdvanceTime: true }) to enable fake timers for Svelte component tests using render. Additionally, mock the window API methods by creating vi.fn() mocks for window.startMcpServer, window.stopMcpServer, and window.showMessageBox, which are asserted throughout the test file including in the error handling test cases.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts`:
- Around line 46-48: The beforeEach hook is missing fake timers configuration
and window API mocks required for the test suite. Add vi.useFakeTimers({
shouldAdvanceTime: true }) to enable fake timers for Svelte component tests
using render. Additionally, mock the window API methods by creating vi.fn()
mocks for window.startMcpServer, window.stopMcpServer, and
window.showMessageBox, which are asserted throughout the test file including in
the error handling test cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e9d14e1f-9745-4774-945a-ba4588dd3fc7
📒 Files selected for processing (2)
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.tspackages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: typecheck
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: Windows
- GitHub Check: unit tests / windows-2022
- GitHub Check: macOS
- GitHub Check: unit tests / macos-15
- GitHub Check: linter, formatters
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: Linux
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts
**/*.spec.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.{ts,tsx,js,jsx}: Usetest()instead ofit()for test cases in Vitest unit tests
Usevi.mock(import('...'))for auto-mocking modules in unit tests; avoid manual mock factories when possible
Usevi.resetAllMocks()inbeforeEachhooks instead ofvi.clearAllMocks()for resetting mocks between tests
When an auto-mocked function or class method needs a real implementation, usevi.mocked(...)with the prototype pattern for class methods:vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)
Files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts
packages/{main,renderer,preload}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Container operations must include
engineIdparameter to identify the container engine
Files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts
🧠 Learnings (6)
📚 Learning: 2026-04-15T08:04:32.031Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1336
File: packages/renderer/src/lib/guided-setup/GuidedSetup.svelte:9-11
Timestamp: 2026-04-15T08:04:32.031Z
Learning: For Svelte components in this repo, if a callback prop is typed as `() => void`, TypeScript idiomatically allows passing async functions (e.g., `() => Promise<void>`), because `() => void` indicates the caller ignores the return value rather than requiring `undefined`. Do not recommend changing these prop types to `() => void | Promise<void>` solely to “fix” async compatibility—unless there is an actual need for the caller to observe the returned value.
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte
📚 Learning: 2026-04-28T13:34:51.610Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1431
File: packages/renderer/src/lib/guided-setup/panels/OpenCodePanel.svelte:14-20
Timestamp: 2026-04-28T13:34:51.610Z
Learning: In this repo’s Svelte renderer (packages/renderer/src/**/*.svelte), `podman-desktop/ui-svelte`’s `Link` component does not accept an `href` prop. For opening external URLs, use the established pattern `on:click={() => window.openExternal(url)}` (optionally typed as `on:click={(): Promise<void> => window.openExternal(url)}`), consistent with existing components like `ProviderLinks.svelte`, `WelcomePage.svelte`, and `OpenCodePanel.svelte`. Do not treat `window.openExternal()` usage in renderer Svelte components as bypassing a security restriction, and do not recommend replacing it with `Link`/`href`-based navigation. (There is also no `setupSecurityRestrictionsOnLinks` utility in the renderer.)
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte
📚 Learning: 2026-04-29T11:54:22.423Z
Learnt from: vancura
Repo: openkaiden/kaiden PR: 1494
File: packages/renderer/src/Loader.svelte:13-13
Timestamp: 2026-04-29T11:54:22.423Z
Learning: In this codebase’s renderer, timer variables in Svelte files (e.g., in `packages/renderer/src/Loader.svelte`) that store `setTimeout` handles are intentionally typed as `NodeJS.Timeout` (and not `ReturnType<typeof setTimeout>`). When reviewing, do not flag or recommend changing this to `ReturnType<typeof setTimeout>` for portability—treat `NodeJS.Timeout` as the established convention for renderer timer variables.
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.sveltepackages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts
📚 Learning: 2026-06-10T15:22:39.639Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 2115
File: packages/renderer/src/lib/models/SemanticRouterCreate.svelte:134-137
Timestamp: 2026-06-10T15:22:39.639Z
Learning: In multi-step wizard UIs, it’s acceptable for “Next step” buttons to be temporarily non-interactive during incremental work delivered across multiple PRs. When reviewing Svelte files under `packages/renderer/src/lib/**`, do not flag these UX issues if the button lacks an onClick handler *and* there is an explicit TODO comment indicating deferred wiring, and the PR author confirms (in the PR description) that this is an intentional staged implementation. If no TODO/deferred-wiring marker (or no author confirmation) is present, treat missing handlers as a potential UX issue.
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte
📚 Learning: 2026-04-15T08:51:08.199Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1336
File: packages/renderer/src/lib/guided-setup/GuidedSetup.spec.ts:64-69
Timestamp: 2026-04-15T08:51:08.199Z
Learning: For Svelte component test files under `packages/renderer/src/**` with names ending in `.spec.ts` that use `render` from `testing-library/svelte`, call `vi.useFakeTimers({ shouldAdvanceTime: true })` inside a `beforeEach`. Keep it in `beforeEach` for these tests to avoid flakiness from Svelte’s internal tick scheduling; do not remove or flag it based on the presence/absence of explicit timer manipulation in individual tests.
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts
🔇 Additional comments (2)
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte (1)
33-40: LGTM!packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts (1)
87-104: LGTM!
bmahabirbu
force-pushed
the
mcp-spawn-fix
branch
from
aa647e2 to
2057a4d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/main/src/plugin/mcp/mcp-manager.ts (1)
236-248:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
addClient()exception-safe after client creation.Line 242 stores the client before Line 244 fetches tools. If
client.tools()rejects,#clientkeeps a live client while the server still looksregistered, andstartMCPServer()can roll storage back toautoSpawn: false. Defer#client.set(...)until tool discovery succeeds, or close/remove the partially created client in a catch before rethrowing.Suggested direction
const wrapped = this.exchanges.createMiddleware(key, transport); const client = await experimental_createMCPClient({ transport: wrapped }); - this.#client.set(key, client); - - const toolSet = await client.tools(); - server.tools = Object.fromEntries( - Object.entries(toolSet).map(([k, v]) => [k, { description: v.description ?? '' }]), - ); - server.status = undefined; + try { + const toolSet = await client.tools(); + this.#client.set(key, client); + server.tools = Object.fromEntries( + Object.entries(toolSet).map(([k, v]) => [k, { description: v.description ?? '' }]), + ); + server.status = undefined; + } catch (err) { + await client.close().catch(() => undefined); + this.exchanges.clearExchanges(key); + throw err; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/main/src/plugin/mcp/mcp-manager.ts` around lines 236 - 248, The addClient() method stores the client in the `#client` map before successfully fetching tools, which leaves the system in an inconsistent state if client.tools() fails. Move the this.#client.set(key, client) call to after the toolSet is successfully retrieved and processed, so the client is only stored in the map once tool discovery completes successfully.
♻️ Duplicate comments (1)
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts (1)
46-49:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd missing window API mocks to prevent test failures.
The
beforeEachhook correctly adds fake timers (line 47) but is still missing mocks forwindow.startMcpServer,window.stopMcpServer, andwindow.showMessageBox. Without these mocks, the tests at lines 76, 85, and 97 will fail at runtime because they assert on these functions. Line 89 explicitly callsvi.mocked(window.startMcpServer), which requires the function to already be mocked.🔧 Proposed fix to add window API mocks
beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.resetAllMocks(); + window.startMcpServer = vi.fn(); + window.stopMcpServer = vi.fn(); + window.showMessageBox = vi.fn(); + window.removeMcpRemoteServer = vi.fn(); + window.exportMcpServer = vi.fn(); + window.getMcpExportConfigPath = vi.fn(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts` around lines 46 - 49, The beforeEach hook is missing mocks for three window API functions that are required by subsequent tests. Add mocks for window.startMcpServer, window.stopMcpServer, and window.showMessageBox within the beforeEach hook (after the fake timers setup and vi.resetAllMocks() call). Use vi.mocked() or vi.stubGlobal() to mock these three functions as vi functions with return values appropriate for your tests, since line 89 and other tests depend on these being mockable with vi.mocked().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/main/src/plugin/mcp/mcp-registry.ts`:
- Around line 683-687: The catch block in startMCPServer() currently rolls back
autoSpawn and rethrows without cleaning up the transport that was spawned before
addClient() was called. When addClient() fails (whether due to a duplicate-start
race or other errors), the spawned process/transport is left running orphaned.
Modify the catch block to distinguish between duplicate-start errors (which
should be treated as success since another caller already started it) and
genuine failures. For genuine failures, clean up the spawned transport instance
before updating the autoSpawn configuration and rethrowing the error. For
duplicate-start races, do not roll back autoSpawn since the server is already
running.
---
Outside diff comments:
In `@packages/main/src/plugin/mcp/mcp-manager.ts`:
- Around line 236-248: The addClient() method stores the client in the `#client`
map before successfully fetching tools, which leaves the system in an
inconsistent state if client.tools() fails. Move the this.#client.set(key,
client) call to after the toolSet is successfully retrieved and processed, so
the client is only stored in the map once tool discovery completes successfully.
---
Duplicate comments:
In `@packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts`:
- Around line 46-49: The beforeEach hook is missing mocks for three window API
functions that are required by subsequent tests. Add mocks for
window.startMcpServer, window.stopMcpServer, and window.showMessageBox within
the beforeEach hook (after the fake timers setup and vi.resetAllMocks() call).
Use vi.mocked() or vi.stubGlobal() to mock these three functions as vi functions
with return values appropriate for your tests, since line 89 and other tests
depend on these being mockable with vi.mocked().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6988120-0045-450c-8cf1-36dfaadcc0be
📒 Files selected for processing (8)
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/main/src/plugin/mcp/mcp-manager.tspackages/main/src/plugin/mcp/mcp-registry.tspackages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.tspackages/renderer/src/lib/mcp/MCPServerRemoteListActions.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: linter, formatters
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: Linux
- GitHub Check: macOS
- GitHub Check: unit tests / macos-15
- GitHub Check: Windows
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / windows-2022
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.tspackages/main/src/plugin/mcp/mcp-registry.tspackages/main/src/plugin/mcp/mcp-manager.ts
packages/main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/main/src/**/*.{ts,tsx}: UseipcHandle()to expose handlers in the main process with naming convention<registry-name>:<action>(e.g.,container-provider-registry:listContainers)
UseapiSender.send()to send events from main process to renderer for real-time updates
Long-running operations should useTaskManager.createTask()with title and action configuration
Files:
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/main/src/plugin/mcp/mcp-registry.tspackages/main/src/plugin/mcp/mcp-manager.ts
packages/main/src/plugin/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
All major services must be registered as singletons in the Inversify DI container during initialization in the PluginSystem
Files:
packages/main/src/plugin/index.ts
packages/{main,renderer,preload}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Container operations must include
engineIdparameter to identify the container engine
Files:
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.tspackages/main/src/plugin/mcp/mcp-registry.tspackages/main/src/plugin/mcp/mcp-manager.ts
**/*.spec.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.{ts,tsx,js,jsx}: Usetest()instead ofit()for test cases in Vitest unit tests
Usevi.mock(import('...'))for auto-mocking modules in unit tests; avoid manual mock factories when possible
Usevi.resetAllMocks()inbeforeEachhooks instead ofvi.clearAllMocks()for resetting mocks between tests
When an auto-mocked function or class method needs a real implementation, usevi.mocked(...)with the prototype pattern for class methods:vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)
Files:
packages/main/src/plugin/mcp/mcp-manager.spec.tspackages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts
🧠 Learnings (8)
📚 Learning: 2026-04-15T08:04:32.031Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1336
File: packages/renderer/src/lib/guided-setup/GuidedSetup.svelte:9-11
Timestamp: 2026-04-15T08:04:32.031Z
Learning: For Svelte components in this repo, if a callback prop is typed as `() => void`, TypeScript idiomatically allows passing async functions (e.g., `() => Promise<void>`), because `() => void` indicates the caller ignores the return value rather than requiring `undefined`. Do not recommend changing these prop types to `() => void | Promise<void>` solely to “fix” async compatibility—unless there is an actual need for the caller to observe the returned value.
Applied to files:
packages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPServerRemoteListActions.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte
📚 Learning: 2026-04-28T13:34:51.610Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1431
File: packages/renderer/src/lib/guided-setup/panels/OpenCodePanel.svelte:14-20
Timestamp: 2026-04-28T13:34:51.610Z
Learning: In this repo’s Svelte renderer (packages/renderer/src/**/*.svelte), `podman-desktop/ui-svelte`’s `Link` component does not accept an `href` prop. For opening external URLs, use the established pattern `on:click={() => window.openExternal(url)}` (optionally typed as `on:click={(): Promise<void> => window.openExternal(url)}`), consistent with existing components like `ProviderLinks.svelte`, `WelcomePage.svelte`, and `OpenCodePanel.svelte`. Do not treat `window.openExternal()` usage in renderer Svelte components as bypassing a security restriction, and do not recommend replacing it with `Link`/`href`-based navigation. (There is also no `setupSecurityRestrictionsOnLinks` utility in the renderer.)
Applied to files:
packages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPServerRemoteListActions.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte
📚 Learning: 2026-04-29T11:54:22.423Z
Learnt from: vancura
Repo: openkaiden/kaiden PR: 1494
File: packages/renderer/src/Loader.svelte:13-13
Timestamp: 2026-04-29T11:54:22.423Z
Learning: In this codebase’s renderer, timer variables in Svelte files (e.g., in `packages/renderer/src/Loader.svelte`) that store `setTimeout` handles are intentionally typed as `NodeJS.Timeout` (and not `ReturnType<typeof setTimeout>`). When reviewing, do not flag or recommend changing this to `ReturnType<typeof setTimeout>` for portability—treat `NodeJS.Timeout` as the established convention for renderer timer variables.
Applied to files:
packages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPServerRemoteListActions.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
packages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPServerRemoteListActions.sveltepackages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/renderer/src/lib/mcp/setup/PackageSetupForm.sveltepackages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.tspackages/main/src/plugin/mcp/mcp-registry.tspackages/main/src/plugin/mcp/mcp-manager.ts
📚 Learning: 2026-06-10T15:22:39.639Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 2115
File: packages/renderer/src/lib/models/SemanticRouterCreate.svelte:134-137
Timestamp: 2026-06-10T15:22:39.639Z
Learning: In multi-step wizard UIs, it’s acceptable for “Next step” buttons to be temporarily non-interactive during incremental work delivered across multiple PRs. When reviewing Svelte files under `packages/renderer/src/lib/**`, do not flag these UX issues if the button lacks an onClick handler *and* there is an explicit TODO comment indicating deferred wiring, and the PR author confirms (in the PR description) that this is an intentional staged implementation. If no TODO/deferred-wiring marker (or no author confirmation) is present, treat missing handlers as a potential UX issue.
Applied to files:
packages/renderer/src/lib/mcp/MCPDetails.sveltepackages/renderer/src/lib/mcp/MCPServerRemoteListActions.sveltepackages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte
📚 Learning: 2026-03-09T08:47:09.657Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 1077
File: packages/main/src/plugin/skill/skill-manager.ts:80-109
Timestamp: 2026-03-09T08:47:09.657Z
Learning: In the kortex-hub/kortex repository, IPC handlers (via ipcHandle()) may be registered directly inside feature manager/service classes (e.g., SkillManager in packages/main/src/plugin/skill/skill-manager.ts) rather than exclusively in packages/main/src/plugin/index.ts. Treat this as an accepted design pattern for files under the plugin directory. Reviewers should not require centralization in index.ts; allow IPC registration proximity to the feature that owns the handler. When reviewing code, accept direct ipcHandle() registrations inside feature managers and ensure the pattern is consistently applied across similar feature-manager modules.
Applied to files:
packages/main/src/plugin/index.tspackages/main/src/plugin/mcp/mcp-manager.spec.tspackages/main/src/plugin/mcp/mcp-registry.tspackages/main/src/plugin/mcp/mcp-manager.ts
📚 Learning: 2026-03-26T08:09:22.450Z
Learnt from: jeffmaury
Repo: kortex-hub/kortex PR: 1169
File: packages/main/src/plugin/mcp/mcp-registry.ts:135-151
Timestamp: 2026-03-26T08:09:22.450Z
Learning: In kortex-hub/kortex (main MCP plugin code), do not rely on `onDidUnregisterRagConnection` for MCP cleanup when tearing down a RAG connection. Per the RAG lifecycle contract, a stop update is always emitted before deletion/unregistration (i.e., `onDidUpdateRagConnection` with status `'stopped'`). Therefore, the `'stopped'` stop handler is sufficient for MCP cleanup; only add `onDidUnregisterRagConnection` if you find a concrete case where unregister can occur without a prior `'stopped'` update.
Applied to files:
packages/main/src/plugin/mcp/mcp-manager.spec.tspackages/main/src/plugin/mcp/mcp-registry.tspackages/main/src/plugin/mcp/mcp-manager.ts
📚 Learning: 2026-04-15T08:51:08.199Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1336
File: packages/renderer/src/lib/guided-setup/GuidedSetup.spec.ts:64-69
Timestamp: 2026-04-15T08:51:08.199Z
Learning: For Svelte component test files under `packages/renderer/src/**` with names ending in `.spec.ts` that use `render` from `testing-library/svelte`, call `vi.useFakeTimers({ shouldAdvanceTime: true })` inside a `beforeEach`. Keep it in `beforeEach` for these tests to avoid flakiness from Svelte’s internal tick scheduling; do not remove or flag it based on the presence/absence of explicit timer manipulation in individual tests.
Applied to files:
packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts
🔇 Additional comments (15)
packages/renderer/src/lib/mcp/MCPDetails.svelte (1)
34-34: LGTM!packages/renderer/src/lib/mcp/MCPServerRemoteListActions.svelte (2)
33-40: LGTM!
106-112: LGTM!packages/renderer/src/lib/mcp/MCPServerRemoteListActions.spec.ts (1)
88-105: LGTM!packages/main/src/plugin/index.ts (4)
145-145: LGTM!
2404-2409: LGTM!
2440-2440: LGTM!
2445-2451: The IPC handler naming is correct and requires no changes.The handlers
mcp-manager:startMCPServerandmcp-manager:stopMCPServerappropriately use themcp-manager:namespace because their underlying implementations (mcpRegistry.startMCPServer()andmcpRegistry.stopMCPServer()) internally orchestrate withmcpManager. Specifically:
startMCPServercallsthis.mcpManager.addClient(key, transport)to manage the client lifecyclestopMCPServercallsthis.mcpManager.removeClient(key)to clean up the clientThe naming is consistent with the established pattern in the file.
> Likely an incorrect or invalid review comment.packages/renderer/src/lib/mcp/setup/PackageSetupForm.svelte (5)
14-21: LGTM!
25-27: LGTM!
79-87: LGTM!
89-97: LGTM!
99-107: LGTM!Also applies to: 181-197
packages/main/src/plugin/mcp/mcp-registry.ts (1)
699-703: LGTM!packages/main/src/plugin/mcp/mcp-manager.spec.ts (1)
103-110: LGTM!
bmahabirbu
force-pushed
the
mcp-spawn-fix
branch
from
2057a4d to
035be0a
Compare
gastoner
reviewed
Jun 16, 2026
| @@ -0,0 +1,124 @@ | |||
| /********************************************************************** | |||
| * Copyright (C) 2025 Red Hat, Inc. | |||
|
|
||
| vi.mock(import('@ai-sdk/mcp')); | ||
|
|
||
| const { experimental_createMCPClient } = await import('@ai-sdk/mcp'); |
Contributor
There was a problem hiding this comment.
Can we import this like import {experimental_createMCPClient} from '@ai-sdk/mcp'?
| await mcpRegistry.stopMCPServer(key); | ||
| }); | ||
|
|
||
| this.ipcHandle( |
Contributor
There was a problem hiding this comment.
Would love to see this moved to mcp-manager init method, but not required
bmahabirbu
force-pushed
the
mcp-spawn-fix
branch
2 times, most recently
from
f371948 to
3431f44
Compare
bmahabirbu
force-pushed
the
mcp-spawn-fix
branch
2 times, most recently
from
b573a7d to
b35776e
Compare
jeffmaury
requested changes
Jun 22, 2026
jeffmaury
left a comment
jeffmaury
left a comment
Contributor
There was a problem hiding this comment.
LGTM but new files should have 2026 not 2025
bmahabirbu
force-pushed
the
mcp-spawn-fix
branch
from
b35776e to
5e69df5
Compare
Package-based MCPs (npm/pypi) can now be registered without spawning a process on the host. This saves the configuration for export to external tools (Claude Code, Cursor, etc.) while keeping the Ready tab clean with status badges showing Connected, Spawned, or Registered. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Brian <bmahabir@bu.edu>
Allow users to start (spawn) and stop registered package MCP servers without removing and re-adding them. Adds addClient/removeClient to MCPManager for in-place state transitions, and updates setupMCPServer and registerMCPServerOnly to toggle state when the server already exists. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Brian <bmahabir@bu.edu>
Display the error message to the user via showMessageBox instead of silently swallowing the error, matching the pattern used by agent workspace start/stop actions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Brian <bmahabir@bu.edu>
…ment Use static import for experimental_createMCPClient in tests and move start/stop MCP server IPC handlers from index.ts to MCPIPCHandler. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Brian <bmahabir@bu.edu>
Match package MCP state by target instead of bare server id so register and spawn actions update the right saved config. Make client startup atomic so failed tool discovery does not leave hidden started state behind. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Brian <bmahabir@bu.edu>
bmahabirbu
force-pushed
the
mcp-spawn-fix
branch
from
5e69df5 to
053cd2d
Compare
jeffmaury
approved these changes
Jun 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Updated video
Screencast.From.2026-06-14.17-17-25.mp4
Fixes: #2144
🤖 Generated with Claude Code