From 3c2123580d1217f9f4e9a56cbdfcb32c97ef19e5 Mon Sep 17 00:00:00 2001 From: muxammadreza <137672463+muxammadreza@users.noreply.github.com> Date: Thu, 25 Jun 2026 11:33:00 -0700 Subject: [PATCH 1/2] fix: make custom server config edits sync immediately --- apps/desktop/src-tauri/src/commands/space.rs | 28 +- .../src/components/ConfigEditorModal.tsx | 314 +++++++++++------- .../src/features/servers/ServersPage.tsx | 1 + .../src/application/user_space_sync.rs | 21 +- 4 files changed, 241 insertions(+), 123 deletions(-) diff --git a/apps/desktop/src-tauri/src/commands/space.rs b/apps/desktop/src-tauri/src/commands/space.rs index a63a08d2..d5e79e1e 100644 --- a/apps/desktop/src-tauri/src/commands/space.rs +++ b/apps/desktop/src-tauri/src/commands/space.rs @@ -6,7 +6,10 @@ //! built-in fallback. The desktop UI tracks which space the user is //! viewing in its own Zustand store (frontend-only state). -use mcpmux_core::{validate_workspace_root, Space, SpaceBaseDir, WorkspaceRootValidation}; +use mcpmux_core::{ + application::UserSpaceSyncService, validate_workspace_root, Space, SpaceBaseDir, + WorkspaceRootValidation, +}; use std::sync::Arc; use tauri::{AppHandle, State}; use tokio::sync::RwLock; @@ -193,7 +196,28 @@ pub async fn save_space_config( serde_json::from_str::(&content) .map_err(|e| format!("Invalid JSON: {}", e))?; - std::fs::write(&config_path, content).map_err(|e| format!("Failed to write config file: {}", e)) + std::fs::write(&config_path, content) + .map_err(|e| format!("Failed to write config file: {}", e))?; + + // Do not rely solely on the debounced file watcher. The UI reloads immediately + // after this command returns, so sync the just-saved file into InstalledServer + // records synchronously to avoid stale/missing custom-server state. + let sync_service = UserSpaceSyncService::new(state.installed_server_repository.clone()); + let sync_result = sync_service + .sync_from_file(&space_id, &config_path) + .await + .map_err(|e| format!("Failed to sync custom server config: {}", e))?; + + if sync_result.has_changes() { + info!( + "[save_space_config] Synced custom server config: {} added, {} updated, {} removed", + sync_result.added.len(), + sync_result.updated.len(), + sync_result.removed.len() + ); + } + + Ok(()) } /// Remove a server from the space configuration file diff --git a/apps/desktop/src/components/ConfigEditorModal.tsx b/apps/desktop/src/components/ConfigEditorModal.tsx index 4c8ffca6..6f62c4d1 100644 --- a/apps/desktop/src/components/ConfigEditorModal.tsx +++ b/apps/desktop/src/components/ConfigEditorModal.tsx @@ -1,5 +1,5 @@ import { useState, useEffect, useCallback, useRef } from 'react'; -import { X, Save, Loader2, AlertTriangle, Wand2 } from 'lucide-react'; +import { X, Save, Loader2, AlertTriangle, Wand2, Plus } from 'lucide-react'; import { readSpaceConfig, saveSpaceConfig } from '@/lib/api/spaces'; import { refreshRegistry } from '@/lib/api/registry'; import Editor, { type Monaco } from '@monaco-editor/react'; @@ -11,11 +11,56 @@ import { RequestServerCTA } from './Contribute'; interface ConfigEditorModalProps { spaceId: string; spaceName: string; + insertNewServer?: boolean; onClose: () => void; onSaved: () => void; } -export function ConfigEditorModal({ spaceId, spaceName, onClose, onSaved }: ConfigEditorModalProps) { +type SpaceConfigJson = { + mcpServers?: Record; + [key: string]: unknown; +}; + +const CUSTOM_SERVER_BASE_KEY = 'custom-server'; + +function nextCustomServerKey(servers: Record): string { + let suffix = 1; + + while (true) { + const key = suffix === 1 ? CUSTOM_SERVER_BASE_KEY : CUSTOM_SERVER_BASE_KEY + '-' + suffix; + if (!(key in servers)) { + return key; + } + suffix += 1; + } +} + +function addCustomServerDraft(config: SpaceConfigJson): SpaceConfigJson { + const mcpServers = { ...(config.mcpServers ?? {}) }; + const key = nextCustomServerKey(mcpServers); + const suffix = + key === CUSTOM_SERVER_BASE_KEY ? '' : ' ' + key.replace(CUSTOM_SERVER_BASE_KEY + '-', ''); + + mcpServers[key] = { + name: 'New Custom Server' + suffix, + command: '', + args: [], + env: {}, + }; + + return { + ...config, + mcpServers, + }; +} + +export function ConfigEditorModal({ + spaceId, + spaceName, + insertNewServer = false, + onClose, + onSaved, +}: ConfigEditorModalProps) { const [content, setContent] = useState(''); const [isLoading, setIsLoading] = useState(true); const [isSaving, setIsSaving] = useState(false); @@ -35,17 +80,19 @@ export function ConfigEditorModal({ spaceId, spaceName, onClose, onSaved }: Conf useEffect(() => { loadConfig(); - }, [spaceId]); + }, [spaceId, insertNewServer]); const loadConfig = async () => { try { setIsLoading(true); setError(null); const data = await readSpaceConfig(spaceId); - // Auto-format on load if valid JSON + // Auto-format on load if valid JSON. When opened from Add Custom Server, + // insert a unique draft entry instead of replacing an existing server block. try { - const parsed = JSON.parse(data); - setContent(JSON.stringify(parsed, null, 2)); + const parsed = JSON.parse(data) as SpaceConfigJson; + const nextConfig = insertNewServer ? addCustomServerDraft(parsed) : parsed; + setContent(JSON.stringify(nextConfig, null, 2)); } catch { setContent(data); } @@ -73,7 +120,7 @@ export function ConfigEditorModal({ spaceId, spaceName, onClose, onSaved }: Conf await saveSpaceConfig(spaceId, content); // Refresh server discovery to pick up new/changed servers await refreshRegistry(); - + success('Configuration saved', 'Space configuration updated successfully'); onSaved(); onClose(); @@ -93,6 +140,20 @@ export function ConfigEditorModal({ spaceId, spaceName, onClose, onSaved }: Conf } }, []); + const handleInsertCustomServer = useCallback(() => { + try { + const parsed = JSON.parse(content || '{"mcpServers":{}}') as SpaceConfigJson; + setContent(JSON.stringify(addCustomServerDraft(parsed), null, 2)); + setIsValidJson(true); + setError(null); + } catch (e) { + const message = e instanceof Error ? e.message : String(e); + setIsValidJson(false); + setError('Invalid JSON: ' + message); + showError('Invalid JSON', message); + } + }, [content, showError]); + // Configure Monaco before mount to set up JSON schema validation const handleEditorBeforeMount = (monaco: Monaco) => { // Configure JSON language with schema validation @@ -114,13 +175,13 @@ export function ConfigEditorModal({ spaceId, spaceName, onClose, onSaved }: Conf const handleEditorMount = (editor: editor.IStandaloneCodeEditor, monaco: Monaco) => { editorRef.current = editor; monacoRef.current = monaco; - + // Focus editor on mount editor.focus(); }; const handleEditorValidation = (markers: editor.IMarker[]) => { - const errors = markers.map(m => `Line ${m.startLineNumber}: ${m.message}`); + const errors = markers.map((m) => `Line ${m.startLineNumber}: ${m.message}`); setValidationErrors(errors); setIsValidJson(markers.length === 0); }; @@ -159,128 +220,141 @@ export function ConfigEditorModal({ spaceId, spaceName, onClose, onSaved }: Conf return ( <> - toasts.find(t => t.id === id)?.onClose(id)} /> -
-
- {/* Header */} -
-
-
- -
-
-

- Custom Server Configuration -

-

- {spaceName} · JSON config -

+ toasts.find((t) => t.id === id)?.onClose(id)} + /> +
+
+ {/* Header */} +
+
+
+ +
+
+

Custom Server Configuration

+

{spaceName} · JSON config

+
+
- -
- {/* Toolbar */} -
- - -
- - - -
- - {!isValidJson && ( - - - {validationErrors.length > 0 ? 'Schema Error' : 'Invalid JSON'} - - )} + {/* Toolbar */} +
+ - - Ctrl+S save · Ctrl+Shift+F format - -
+
+ + + + + +
+ + {!isValidJson && ( + + + {validationErrors.length > 0 ? 'Schema Error' : 'Invalid JSON'} + + )} - {/* Contribute / Request CTA — surfaces the registry templates so users + + Ctrl+S save · Ctrl+Shift+F format + +
+ + {/* Contribute / Request CTA — surfaces the registry templates so users don't have to hand-roll a definition if one already exists upstream. */} -
- -
+
+ +
- {/* Editor Area */} -
- {(isLoading || !editorReady) ? ( -
- + {/* Editor Area */} +
+ {isLoading || !editorReady ? ( +
+ +
+ ) : ( + + +
+ } + /> + )} +
+ + {/* Footer / Status Bar */} + {(error || validationErrors.length > 0) && ( +
+ {error || validationErrors.slice(0, 3).join(' • ')} + {validationErrors.length > 3 && ` (+${validationErrors.length - 3} more)`}
- ) : ( - - -
- } - /> )}
- - {/* Footer / Status Bar */} - {(error || validationErrors.length > 0) && ( -
- {error || validationErrors.slice(0, 3).join(' • ')} - {validationErrors.length > 3 && ` (+${validationErrors.length - 3} more)`} -
- )}
-
); } diff --git a/apps/desktop/src/features/servers/ServersPage.tsx b/apps/desktop/src/features/servers/ServersPage.tsx index 8db8b895..70117f44 100644 --- a/apps/desktop/src/features/servers/ServersPage.tsx +++ b/apps/desktop/src/features/servers/ServersPage.tsx @@ -1836,6 +1836,7 @@ export function ServersPage() { setEditConfigSpace(null)} onSaved={() => { loadData(); // Reload servers after config save diff --git a/crates/mcpmux-core/src/application/user_space_sync.rs b/crates/mcpmux-core/src/application/user_space_sync.rs index 1e0e240c..825b6957 100644 --- a/crates/mcpmux-core/src/application/user_space_sync.rs +++ b/crates/mcpmux-core/src/application/user_space_sync.rs @@ -3,7 +3,7 @@ //! Syncs servers from user space JSON configuration files into InstalledServer records. //! This enables a unified connection flow regardless of server source (Registry vs UserConfig). -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::path::Path; use std::sync::Arc; @@ -74,6 +74,25 @@ impl UserSpaceSyncService { // 2. Convert to ServerDefinitions let definitions = config.to_server_definitions(space_id, file_path.to_path_buf()); + + // User-config keys are normalized into MCP-safe server IDs. Do not allow + // two entries to collapse to the same ID; that would make the sync loop + // update the same InstalledServer row and appear to overwrite the previous + // custom server. + let mut seen_ids: HashMap = HashMap::new(); + for definition in &definitions { + if let Some(first_name) = + seen_ids.insert(definition.id.clone(), definition.name.clone()) + { + anyhow::bail!( + "Multiple custom servers normalize to the same id '{}': '{}' and '{}'. Rename one mcpServers key to a distinct alphanumeric/hyphen/dot id.", + definition.id, + first_name, + definition.name + ); + } + } + let file_server_ids: HashSet = definitions.iter().map(|d| d.id.clone()).collect(); debug!( From 54d531dd8e2dabd096c2c8a3a2119a846a94b023 Mon Sep 17 00:00:00 2001 From: Mohammod Al Amin Ashik Date: Sat, 27 Jun 2026 13:36:14 +0800 Subject: [PATCH 2/2] test(core): cover the custom-server id-collision guard Extracts the duplicate-normalized-id check in UserSpaceSyncService into `ensure_unique_server_ids` and adds unit tests: two mcpServers keys that normalize to the same id (e.g. "My Server" / "my_server" -> "myserver") are rejected with a clear error, and distinct ids pass. Covers the collision branch without needing a repo or an on-disk file. Signed-off-by: Mohammod Al Amin Ashik --- .../src/application/user_space_sync.rs | 81 ++++++++++++++----- 1 file changed, 63 insertions(+), 18 deletions(-) diff --git a/crates/mcpmux-core/src/application/user_space_sync.rs b/crates/mcpmux-core/src/application/user_space_sync.rs index 825b6957..90848a89 100644 --- a/crates/mcpmux-core/src/application/user_space_sync.rs +++ b/crates/mcpmux-core/src/application/user_space_sync.rs @@ -11,7 +11,7 @@ use anyhow::{Context, Result}; use tracing::{debug, info}; use crate::domain::config::UserSpaceConfig; -use crate::domain::{InstallationSource, InstalledServer}; +use crate::domain::{InstallationSource, InstalledServer, ServerDefinition}; use crate::repository::InstalledServerRepository; /// Result of a sync operation @@ -48,6 +48,29 @@ impl UserSpaceSyncService { Self { installed_repo } } + /// Ensure no two user-config entries normalize to the same MCP server id. + /// + /// User-config keys are normalized into MCP-safe server ids; if two entries + /// collapse to the same id the sync loop would update the same + /// `InstalledServer` row and appear to overwrite the previous custom server. + /// Reject that up front with a clear error instead of silently dropping one. + fn ensure_unique_server_ids(definitions: &[ServerDefinition]) -> Result<()> { + let mut seen_ids: HashMap = HashMap::new(); + for definition in definitions { + if let Some(first_name) = + seen_ids.insert(definition.id.clone(), definition.name.clone()) + { + anyhow::bail!( + "Multiple custom servers normalize to the same id '{}': '{}' and '{}'. Rename one mcpServers key to a distinct alphanumeric/hyphen/dot id.", + definition.id, + first_name, + definition.name + ); + } + } + Ok(()) + } + /// Sync servers from a user space JSON file into InstalledServer records /// /// This performs a 3-way diff: @@ -75,23 +98,10 @@ impl UserSpaceSyncService { // 2. Convert to ServerDefinitions let definitions = config.to_server_definitions(space_id, file_path.to_path_buf()); - // User-config keys are normalized into MCP-safe server IDs. Do not allow - // two entries to collapse to the same ID; that would make the sync loop - // update the same InstalledServer row and appear to overwrite the previous - // custom server. - let mut seen_ids: HashMap = HashMap::new(); - for definition in &definitions { - if let Some(first_name) = - seen_ids.insert(definition.id.clone(), definition.name.clone()) - { - anyhow::bail!( - "Multiple custom servers normalize to the same id '{}': '{}' and '{}'. Rename one mcpServers key to a distinct alphanumeric/hyphen/dot id.", - definition.id, - first_name, - definition.name - ); - } - } + // User-config keys are normalized into MCP-safe server IDs; reject two + // entries that collapse to the same ID up front so the sync loop can't + // silently overwrite one custom server with another. + Self::ensure_unique_server_ids(&definitions)?; let file_server_ids: HashSet = definitions.iter().map(|d| d.id.clone()).collect(); @@ -252,4 +262,39 @@ mod tests { assert_eq!(result.total_changes(), 3); } + + fn definitions_from(json: &str) -> Vec { + let config: UserSpaceConfig = serde_json::from_str(json).expect("valid config json"); + config.to_server_definitions("space-1", std::path::PathBuf::from("test.json")) + } + + #[test] + fn ensure_unique_server_ids_rejects_colliding_normalized_ids() { + // "My Server" and "my_server" both normalize to "myserver". + let definitions = definitions_from( + r#"{ "mcpServers": { + "My Server": { "command": "echo" }, + "my_server": { "command": "echo" } + } }"#, + ); + + let err = UserSpaceSyncService::ensure_unique_server_ids(&definitions) + .expect_err("colliding normalized ids must be rejected"); + assert!( + err.to_string().contains("myserver"), + "error should name the colliding id, got: {err}" + ); + } + + #[test] + fn ensure_unique_server_ids_accepts_distinct_ids() { + let definitions = definitions_from( + r#"{ "mcpServers": { + "alpha": { "command": "echo" }, + "beta": { "command": "echo" } + } }"#, + ); + + assert!(UserSpaceSyncService::ensure_unique_server_ids(&definitions).is_ok()); + } }