Claude/mcp UI features

[jasonkneen]

• Add MCP server connection and tool integration
• Create MCP client for JSON-RPC communication with servers
• Spawn MCP server processes with configurable command/args/env
• Implement initialize, list_tools, and call_tool methods
• Connect to enabled MCP servers on app startup
• Reconnect MCP servers when settings change
• Add IPC handlers for getting and executing MCP tools
• Disconnect all MCP servers on app quit
• Fix MCP server args by sanitizing quotes
• Strip surrounding quotes from args before spawning MCP server process.

[Copilot]

Pull request overview

This PR adds comprehensive Model Context Protocol (MCP) server integration to Open Claude, enabling external tool connectivity and multi-window/multi-tab support. The implementation includes a complete UI for managing MCP servers, connection lifecycle management, and tool selection interface.

Key Changes

• MCP Integration: Full MCP SDK integration with server lifecycle management (connect, disconnect, reconnect)
• UI Enhancements: Settings page for MCP server configuration, tools popup for tool selection, tab management with drag-to-detach functionality
• Multi-Window Support: Window management system allowing tab detachment and multiple application windows

Reviewed changes

Copilot reviewed 11 out of 13 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
static/styles/settings.css	Extensive CSS for MCP server UI including modals, server cards, tool badges, and dark mode support
static/settings.html	Added MCP servers section with server list and add/edit modal
static/index.html	Added tab bar, tools popup, sidebar resize, and updated styles for multi-window UI
src/types/index.ts	Added MCPServerConfig interface to settings schema
src/renderer/settings.ts	MCP server CRUD operations and UI rendering logic
src/renderer/main.ts	Tab management, MCP tools popup, window communication, and sidebar resizing
src/preload.ts	IPC bridge for MCP and window management APIs
src/mcp/client.ts	New MCP client wrapper handling stdio/SSE transports and tool execution
src/main.ts	Main process handlers for MCP operations, window management, and macOS permissions
pnpm-lock.yaml	Added @modelcontextprotocol/sdk and dependencies
package.json	Added MCP SDK dependency and macOS entitlements configuration
build/entitlements.mac.plist	New macOS entitlements for camera, microphone, screen capture, and automation

Files not reviewed (1)

• pnpm-lock.yaml: Language not supported

Comments suppressed due to low confidence (1)

build/entitlements.mac.plist:23

• Security concern: The entitlements grant broad permissions including JIT, unsigned executable memory, and library validation disabling. While these may be needed for Electron development, they weaken security significantly. Ensure these are absolutely necessary for the MCP integration and consider removing them for production builds if possible. Specifically:

• com.apple.security.cs.allow-jit
• com.apple.security.cs.allow-unsigned-executable-memory
• com.apple.security.cs.disable-library-validation
• com.apple.security.cs.allow-dyld-environment-variables

These entitlements should be documented with clear justification.

    <!-- Allow app to be debugged -->
    <key>com.apple.security.cs.allow-jit</key>
    <true/>
    <key>com.apple.security.cs.allow-unsigned-executable-memory</key>
    <true/>
    <key>com.apple.security.cs.disable-library-validation</key>
    <true/>

    <!-- Camera access -->
    <key>com.apple.security.device.camera</key>
    <true/>

    <!-- Microphone access -->
    <key>com.apple.security.device.audio-input</key>
    <true/>

    <!-- Screen recording (required for screen capture) -->
    <key>com.apple.security.cs.allow-dyld-environment-variables</key>
    <true/>

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Similar escapeHtml function is defined here as well. This is code duplication - consider extracting this to a shared utility module that both settings.ts and main.ts can import to maintain DRY principles.

[Copilot]

The comment on line 21 is misleading. The entitlement com.apple.security.cs.allow-dyld-environment-variables is not the correct entitlement for screen recording. Screen recording on macOS doesn't have a specific entitlement - it's controlled by system permissions (TCC). This entitlement allows the app to use DYLD environment variables which is a security risk. Remove this or correct the comment and entitlement.

Suggested change

	<!-- Screen recording (required for screen capture) -->
	<key>com.apple.security.cs.allow-dyld-environment-variables</key>
	<true/>

[Copilot]

Race condition: The drag-and-drop tab detachment checks tabs.length > 1 at the start of dragend, but by the time the async detachTab completes and closeTab is called, another tab could have been closed. This could result in the last tab being closed. Store the length check result before the async call or re-check after.

Suggested change

	// Remove the tab from this window
	closeTab(tabId);
	}
	// Remove the tab from this window, but only if more than one tab remains
	if (tabs.length > 1) {
	closeTab(tabId);
	}

[Copilot]

Missing error handling for window.claude.detachTab(). If the IPC call fails, the tab will remain in this window but the operation will silently fail. Add error handling:

try {
  await window.claude.detachTab({...});
  closeTab(tabId);
} catch (error) {
  console.error('Failed to detach tab:', error);
  // Show error to user
}

[Copilot]

The tool name format used for Claude API (mcp_${serverName}_${tool.name}) should be documented and may cause collisions if a server name or tool name contains underscores. Consider using a more robust delimiter or encoding scheme, or document this limitation in the code comments.

Suggested change

	tools.push({
	name: `mcp_${serverName}_${tool.name}`,
	// Tool name format for Claude API: mcp_{encodedServerName}_{encodedToolName}
	// Both serverName and tool.name are URI-encoded to avoid collisions with underscores or other special characters.
	tools.push({
	name: `mcp_${encodeURIComponent(serverName)}_${encodeURIComponent(tool.name)}`,

[Copilot]

[nitpick] The function escapeHtml is defined but could be replaced with a standard library or more comprehensive escaping. The current implementation only escapes through textContent assignment, which is safe but the function name suggests it returns an escaped string. Consider using a dedicated library like DOMPurify or he for consistent HTML escaping, or rename the function to sanitizeForHtml to better reflect its behavior.

[Copilot]

The env property is typed as Record<string, string> | undefined but is being spread with process.env which may contain undefined values. This could cause type errors since process.env values can be string | undefined. Consider filtering undefined values or explicitly typing the result.

env: config.env ? { ...process.env, ...config.env } as Record<string, string> : process.env as Record<string, string>

Suggested change

	env: config.env ? { ...process.env, ...config.env } as Record<string, string> : undefined,
	env: config.env
	? {
	...Object.fromEntries(
	Object.entries(process.env).filter(([_, v]) => typeof v === 'string')
	),
	...config.env
	}
	: undefined,

[Copilot]

The parsing logic assumes the tool name follows the pattern mcp_serverName_toolName and uses parts.slice(2).join('_') to reconstruct the tool name. This means if the original tool name contained underscores, they would be preserved correctly. However, if a server name contains underscores, this parsing will fail. Consider documenting this constraint or validating server names to disallow underscores.

Suggested change

	// Validate that serverName does not contain underscores
	if (serverName.includes('_')) {
	throw new Error(`Invalid MCP server name: "${serverName}". Server names must not contain underscores.`);
	}

[Copilot]

The will-quit event handler is async but the event doesn't wait for async operations to complete. This means mcpClient.disconnectAll() may not finish before the app quits, potentially leaving processes running. Use e.preventDefault() and app.quit() pattern to ensure cleanup completes.

app.on('will-quit', (e) => {
  e.preventDefault();
  globalShortcut.unregisterAll();
  mcpClient.disconnectAll().then(() => {
    app.exit(0);
  });
});

Suggested change

	app.on('will-quit', async () => {
	globalShortcut.unregisterAll();
	await mcpClient.disconnectAll();
	app.on('will-quit', (e) => {
	e.preventDefault();
	globalShortcut.unregisterAll();
	mcpClient.disconnectAll().then(() => {
	app.exit(0);
	});

[Copilot]

This entitlements file enables several hardened runtime exceptions (com.apple.security.cs.allow-jit, com.apple.security.cs.allow-unsigned-executable-memory, com.apple.security.cs.disable-library-validation, and com.apple.security.cs.allow-dyld-environment-variables) for the production macOS build. These flags effectively disable key code integrity protections and allow arbitrary unsigned or injected code (e.g., via DYLD_INSERT_LIBRARIES) to run inside the Electron app with its full camera/mic/files/network/automation privileges, which a local attacker or malware can abuse. For production builds, these exceptions should be removed or restricted to a separate debug-only entitlements file so that release binaries keep library validation, W^X protections, and DYLD environment variable restrictions enabled.